Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvals to function calls #81

Open
Try opened this issue Aug 6, 2023 · 4 comments
Open

Improvals to function calls #81

Try opened this issue Aug 6, 2023 · 4 comments
Labels
enhancement New feature or request modded Issues or features related to modifications of Gothic

Comments

@Try
Copy link
Contributor

Try commented Aug 6, 2023

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 lmichaelis added enhancement New feature or request modded Issues or features related to modifications of Gothic labels Aug 7, 2023
@lmichaelis
Copy link
Member

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
Copy link
Contributor Author

Try commented Aug 7, 2023

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
Copy link
Member

lmichaelis commented Aug 7, 2023

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
Copy link
Contributor Author

Try commented Aug 7, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request modded Issues or features related to modifications of Gothic
Projects
None yet
Development

No branches or pull requests

2 participants