GothicKit / ZenKit

A re-implementation of file formats used by the early 2000's ZenGin
http://zk.gothickit.dev/
MIT License
44 stars 9 forks source link

Improvals to function calls #81

Open Try opened 11 months ago

Try commented 11 months ago

Some issues with functions

  1. Scripts compiled by vanilla compiler may have inconsistent sequence of push/pop, leaving more data in stack, that expected by caller Proposal: improve guard-related code in opcode::bl

  2. Need to have traps for call-exit. Two use-cases here: a) exit from loop-body:

    BUCK = _^(BUCKET);
    REPEAT(I, (BUCK.NUMINARRAY) / (2)); // opengothic tracks repeat/while loops and memorizes pointer to I, num iterations
    if ((MEM_ARRAYREAD(BUCKET, (I) * (2))) == (KEY)) {
    return MEM_ARRAYREAD(BUCKET, ((I) * (2)) + (1)); // ...and tracking fails here
    };
    END;

    b) LeGo locals package: locals allow to annotate some functions to have proper local-variables. On side of opengothic I can trap annotation just fine and stash 'local' variables, but need to have a way to restore previous values on function exit

PS: can do it after finishing with memory-traps PR :)

lmichaelis commented 11 months ago

Scripts compiled by vanilla compiler may have inconsistent sequence of push/pop, leaving more data in stack, that expected by caller Proposal: improve guard-related code in opcode::bl

Can you point out an instance in which this causes problems? Also it should probably not be handled by the opcode handler in exec but in the unsafe_call function since that is what is ultimately responsible for that. It should also be an execution flag for the VM.

Try commented 11 months ago

Can you point out an instance in which this causes problems?

Tested vanilla G2-notr, via:

auto sz = _m_stack_ptr;
sz += (sym->has_return() ? 1 : 0);
sz -= (sym->count());
unsafe_call(sym);
if (sz != _m_stack_ptr) {
    static std::unordered_set<const symbol*> sx;
    if (sx.find(sym) == sx.end()) {
        sx.insert(sym);
        PX_LOGE("bad stack: ", sym->name());
    }

found some functions:

[phoenix] bad stack: B_MM_DESYNCHRONIZE   // this one I double checked manually, but not the rest
[phoenix] bad stack: B_CLEARRUNEINV
[phoenix] bad stack: B_GIVETRADEINV
[phoenix] bad stack: B_DRAGONKILLCOUNTER

It should also be an execution flag for the VM.

Are you sure? Hard to imagine case, when corrupted stack is desired way of running the script.

lmichaelis commented 11 months ago

Tested vanilla G2-notr, via:

Yes I know these cases exist, but where does extra data on the stack cause actual problems? Just so I can properly understand why this is needed :)

Are you sure? Hard to imagine case, when corrupted stack is desired way of running the script.

It's more of a thing about accurately running the code. For example, it is conceivable that someone might implement a compiler with support for multiple return types in which case this change would break their implementation even though they did generate valid Daedalus bytecode. There are probably other cases in which things might break too.

Try commented 11 months ago

Yes I know these cases exist, but where does extra data on the stack cause actual problems?

Lack of data - problem in CoM. Getting "popping instance from empty stack" message. Extra data, in theory - yes, if something like foo(boo(), ext_data()) - in such case boo output would be overridden.

It's more of a thing about accurately running the code.

OK, will do with flag. But dough that compiler like that will come any time soon ;)