[Scummvm-devel] Dreamweb engine inclusion.

[Max Horn]

Am 15.06.2011 um 15:39 schrieb Eugene Sandulenko:

> On 14 June 2011 15:13, Max Horn <max at quendi.de> wrote:
>> As such, I have serious doubts that releasing this engine under the GPL is legally possible, as long as the original creators do not agree to let their ASM code be published. (I actually wonder why those folks don't want that. Does it contain IP they don't own or what? Are they ashamed of it? )
> OK, gentlemen, everything got resolved gracefully:
> 
>> Neil Jay Dodwell
>> I don't see any reason why the asm code can't be published under GPL.

Excellent :)

> 
> So we are putting the original code next to the tasmrecover utility.
> We will put only the engine, without the game editor.
> 
> Also since I saw no objections to the engine itself, I am giving a
> green light to the merge tonight, so we will continue development
> right in the tree.

Anyway, this means I can now send the list of remarks on the engine I cooked up previously but never finished, as the engine was in limbo ;). 

First off, I assume that with the engine being merged, fixing any warnings triggered by the generated code will become a priority. Recognizing unused labels could also be combined with recognizing (and removing) dead code, such as the code following the label "alreadyloaded" in line 21414.


Also, the generated code should comply to our code formatting guidelines. E.g. "stack_depth" should become "stackDepth", 
  void textforend(Context & context) {
should become
  void textforend(Context &context) {
etc. -- I assume this will will be trivial to achieved :)


Next, I think it would be still be of great use if some effort was spent on making the generated code cleaner / more "human readable", for I assume a lot of debugging will be using it (simply because that's where gdb & valgrind will have to work with)... But then again, maybe I am alone with this ?

These 22,000 lines of auto-generated code still makes my skin crawl *sigh*. Don't get me wrong: This whole "asm to C" idea is pretty awesome and quite a cool feat! Hats off!

However, the thought of having to debug using it (and as I mentioned above, that's where debugging would have to start if using gdb) seems quite painful. As a matter of fact, if I was working on this, my goal wouldn't be to tune the asm->C converter as much as possible to produce a result that is faithful yet readable enough; and once I've achieved that to my satisfaction, would at some point abandon the asm and turn to fine tuning the generated code by hand. But to my surprise, it seems people prefer to work with the extra indirection level. Well, whatever suits you...

Still, could we try to at least teach the decompiler to recognize simple "if/else" or "switch" constructs? I imagine that the simple tricks used in descumm would suffice for many cases. And since the input is hand-written ASM, it probably is easier to treat than the usual compiler generated garbage (of course human beings sometimes employ ingenious tricks that are even "weirder" than anything a compiler produces, but in my experience, most good asm coders produce quite legible asm)

For example, let's look at setuptimeduse() in line 14136. This currently has this code:
	context._cmp(context.data.word(kTimecount), 0);
	if (!context.flags.z()) goto cantsetup;
        ...
cantsetup:
	{assert(stack_depth == context.stack.size()); return; }

A first cheap improvement (IMHO) would be to replace this by
	if (!context._cmp(context.data.word(kTimecount), 0)) goto cantsetup;
        ...
cantsetup:
	{assert(stack_depth == context.stack.size()); return; }
 
assuming that _cmp is changed to return flags.z(). But in fact, with some very simple logic (the same used in descumm), this simple instance could be changed to
	if (context._cmp(context.data.word(kTimecount), 0)) {
          ...
        }
	{assert(stack_depth == context.stack.size()); return; }
Of course some further code analysis could then change this to
	if (context.data.word(kTimecount) == 0) {
          ...
        }
	{assert(stack_depth == context.stack.size()); return; }
but this then is already more difficult, as doing it correctly would require performing analysis on the CPU flags z,c,s,o. This still isn't that hard for many simple cases, *IF* one postulates that flag changes are *not* relevant after a function exits. I am not sure if this is a valid assumption here, but it wouldn't be hard to tweak the code to check that theory, by adding a third state for every flag: undefined. when a function returns, the flags are set to "undefined"; trying to use an undefined flag would trigger an error.
Similarly, I wonder if functions may "return" values in any of the registers? If not (resp. if only a few registers are used for that), this could be used to further improve the generated code. 

An alternative would be to let byte() return a proxy object (like word() already does), and then equip this with overloaded operators ==, !=, <=, <  etc. that internally call _cmp. 



* There is a 10kb binary blob called "src" inside __start, which contains the initial "data heap", and as such initial values for many "variables" and stuff. I wonder if this could be turned into something more readable?

It also seems (I guess?) to contained tables of function offsets, which are used as jump tables in combination with __dispatch_call. Right? My hope would be that this binary blob could be made at least *somewhat* more readable. Assuming that it isn't a blob in the original source -- or is it?



* The stack checking could be made more C++ "style" as follows. Consider this code fragment:

void someFunction(Context &context) {
  uint stack_depth = context.stack.size();
...
label:
  {assert(stack_depth == context.stack.size()); return; }
...
  {assert(stack_depth == context.stack.size()); return; }
}

The stach check above could be improved with a stack based C++ object. So the two lines above would become
  STACK_CHECK(context);
  ...
label:
  return;
....
  return;

and we would have this in a common header:
  #ifdef NDEBUG
  #define STACK_CHECK(context)  StackChecker checker(context)
  #else
  #define STACK_CHECK(context)  do {} while (0)
  #endif

  class StackChecker {
    const uint _stackDepth;
  public:
    StackChecker(const Context & context) : _stackDepth(context.stack.size()) {}
    ~StackChecker() { assert(stack_depth == context.stack.size()); }
  };

The advantage is (IMHO) that this makes the code more readable, and perhaps also will make it simpler to get rid of some more unnecessary "return" statements.


* As I understand, it is possible to tell the converter to ignore certain functions and use hand-coded alternatives for them. Could this be used to replace width160() ? All it does is call
 	context._movsw();
exactly 162 times... So this could be replaced by a loop, or even better, a memcpy plus some di/si adjustments (and OOB checks).


Cheers,
Max