ZenKit icon indicating copy to clipboard operation
ZenKit copied to clipboard

Improvals to function calls

Open Try opened this issue 2 years ago • 4 comments

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 :)

Try avatar Aug 06 '23 17:08 Try

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.

lmichaelis avatar Aug 07 '23 15:08 lmichaelis

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.

Try avatar Aug 07 '23 18:08 Try

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.

lmichaelis avatar Aug 07 '23 18:08 lmichaelis

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 ;)

Try avatar Aug 07 '23 20:08 Try