Skip to content

Global variable that wasn't used before is not referenced correctly when passed to function as parameter. #131

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

Closed
Yiin opened this issue Jan 20, 2017 · 15 comments

Comments

@Yiin
Copy link

Yiin commented Jan 20, 2017

Sample code:

#include <a_samp>

new foo = 1;

public OnFilterScriptInit() {
	test(foo);
}

test(var) {
	printf("%i", var); // 37
}

If I print something before calling test function, character code is passed instead:

#include <a_samp>

new foo = 1;

public OnFilterScriptInit() {
	printf("A");
	test(foo);
}

test(var) {
	printf("%i", var); // 65
}

and finally if I modify value of that global variable, everything works just fine:

#include <a_samp>

new foo = 1;

public OnFilterScriptInit() {
	foo++;
	test(foo);
}

test(var) {
	printf("%i", var); // 2
}
@Yiin Yiin changed the title When I pass global variable that still has it's initial value to function, it doesn't work as expected. Global variable that wasnt used before is not referenced correctly when passed to function as parameter. Jan 20, 2017
@Yiin Yiin changed the title Global variable that wasnt used before is not referenced correctly when passed to function as parameter. Global variable that wasn't used before is not referenced correctly when passed to function as parameter. Jan 20, 2017
@YashasSamaga
Copy link
Member

YashasSamaga commented Jan 20, 2017

I tried with foo = 43 (0x2B).

If I don't access foo before calling test, then the compiler does not store 2B (or the variable) in the AMX. If I use foo, then you can find 2B in the AMX. There is also 0x25 (37) stored adjacent to the foo default value which explains why you get 37 when foo isn't used.

The compiler removes the variable from the AMX if it is not directly accessed (foo++ or foo--, etc). The compiler does write the correct assembly instruction to push foo but foo does not exist. The value which takes its place is 37.

The following code prints the correct value, i.e.: 43.

#include <a_samp>

new foo = 43;

main()
{
	new b = foo;
	test(foo);
}

test(var) {
	printf("%d", var); // 37
}

The bug is somewhere in function argument code. The compiler does not realize that foo was passed as an argument. Since nowhere else the variable foo is used, it assumes that it was never used. Interestingly, it does not trigger a warning that it was unused.

@Yiin
Copy link
Author

Yiin commented Jan 20, 2017

for now my work around is

main() {
	#pragma warning push
	#pragma warning disable 215
	foo;
	#pragma warning pop
        test(foo);
}

@Y-Less
Copy link
Member

Y-Less commented Jan 20, 2017

I think you might have just solved a strange problem I had once and never managed to find the cause of (I stopped it with some random extra states and variables around the place, but wasn't sure how they helped). Thank you!

I'm slightly amazed this hasn't come up more often (or, indeed, at all).

I did some more testing - the code works if your global is declared const (because it is inlined), but not static, public, or new; on both the old and new compiler (so it isn't, say, a regression from the changes to unused publics). Stangely, it also works if the global is passed directly to a native function not a user function so a possibly simpler work-around is:

#define FIX_GLOBAL(%0) clamp(_:%0)

main() {
	FIX_GLOBAL(foo);
        test(foo);
}

Or at least it is simpler to type, more portable between compiler versions, and looks more obvious as to what you are trying to do IMHO. I'm using clamp there because it has no side-effects and does almost nothing, especially when min and max are their defaults. You could even do this (but it would rely on the new compiler getting a version bump after this is fixed):

#if __Pawn <= 0x030A
	#define FIX_GLOBAL(%0) clamp(_:%0)
#else
	#define FIX_GLOBAL(%0);
#endif

main() {
	FIX_GLOBAL(foo);
        test(foo);
}

Which gives me another idea...

@Zeex
Copy link
Contributor

Zeex commented Jan 21, 2017

I tried to debug this today as I was curious too, found that the compiler throws foo away when it checks what arguments are passed to test() vs. what parameters it has because it doesn't have information about parameters at that point. I goes like this:

  • First pass:
    • declglb() parses foo as a global variable and detects that it's not marked as used, so skips it
    • callfunction() parses the call test(foo) and determines that test() doesn't have any arguments so throws an error about argument mismatch, which is silently swallowed because it's not a write pass
  • Second pass:
    • declglb() parses foo again and it's still not marked as used, so skips it for good
    • callfunction() parses the call test(foo) but it's already too late to mark foo as used (a fix which I tried initially)

It looks like when a function is used before it's declaration/definition is parsed we need to cause a re-parse (via sc_reparse?), but not sure if it will work and/or a good idea.

Update:

Nope, can't do a reparse from there, sc_reparse is not accessible from that file. Could be exposed as a global but it feels wrong. There must be a better way to do this. Maybe process global variables after functions?

@Zeex Zeex closed this as completed in 5bedccc Jan 21, 2017
@Zeex
Copy link
Contributor

Zeex commented Jan 21, 2017

It should work now I think, at least it worked for the test code. The compiler will now make an additional pass when some function is used before it's declared, same as with tagged functions. It doesn't issue a warning though, I figured it would be too much warnings.

@Y-Less
Copy link
Member

Y-Less commented Jan 21, 2017

Does it still warn if you use tagged functions before they are declared, or is that just gone as well. I suspect that was from the days of much slower computers where a third pass was a potential problem; although I've seen vast modes where it still is a problem.

@Zeex
Copy link
Contributor

Zeex commented Jan 21, 2017

It will not warn about tagged functions either, just seemed weird if it would continue doing that but would shut up when it comes to non-tagged ones.

But I suspect this can significantly affect compile speed with large enough scripts, do you think people should be aware of it?

@Y-Less
Copy link
Member

Y-Less commented Jan 21, 2017

I have seen multi-minute builds, increasing them all 50% for one tiny fix for an issue almost never seen doesn't seem worth it (though I can understand the desire for a simple fix). I'm not sure how your fix works though - does it add an extra pass for any function used before it is declared, or only those with unseen global variables in the parameters? If the latter (i.e. specifically targeting only this bug) then a warning is probably appropriate as it will rarely ever appear and when it does they should know why their compile times just plummeted. If it adds extra passes in other cases as well (the former), that shouldn't be silent (and IMHO shouldn't happen at all) because of the serious degredation for the vast majority of scripts that never have this problem.

@Zeex Zeex reopened this Jan 21, 2017
@Zeex
Copy link
Contributor

Zeex commented Jan 21, 2017

OK, it sucks then, need something better. I'll leave this issue open.

@ghost
Copy link

ghost commented Jan 21, 2017

I saw that there was a discussion about compiler speed. It's not the best place to write, but I've found a FUCKING FAST compiler at gtaforums, I don't know what "nonamenoname" did with his version, but it works - might you're interested in. My mode before took 3 minute to compile, now 6 second. OMG

http://gtaforums.com/topic/760017-relsa-samp-addon/page-9#entry1069208196

@Y-Less
Copy link
Member

Y-Less commented Jan 21, 2017

I've tested that one, and it might work on some modes, but on many others it doesn't. I don't know what he did either, but I suspect it was deleting some uncommon, but still sometimes useful while slow, features.

It would be good if it was open-source so the improvements could hopefully be integrated and fixed.

@SysadminJeroen
Copy link
Contributor

It appears to be a modified version of Zeex's pawn compiler. With some modifications (that make it speedy af) and it looks like it has Russian translations for error codes.

I have already sent an email on 13 december asking kindly if they could release the source code. I have not received a reply yet.

@Zeex
Copy link
Contributor

Zeex commented Jan 22, 2017

Another possible workaround:

main() {
	test(foo+0);
}

If the variable is part of a constant expression it's marked as read while parsing the expression, so you can just add something to it, i.e. zero.

Or using an enum to store all globals:

enum Globals {
	FOO,
        BAR
}

new g[Globals] = {0xABABABAB};

main() {
	test(g[FOO]);
}

@Zeex Zeex closed this as completed in 25b21eb Jan 22, 2017
Zeex added a commit that referenced this issue Jan 28, 2017
@WoutProvost
Copy link

I did some more testing - the code works if your global is declared const (because it is inlined), but not static, public, or new; on both the old and new compiler (so it isn't, say, a regression from the changes to unused publics). Stangely, it also works if the global is passed directly to a native function not a user function so a possibly simpler work-around is ...

OMG, I am so glad I came across this!
I had some really strange compiler behaviour regarding this. However, the piece of code where the crash occurs hasn't been changed for a long time, so I don't know why the compiler suddenly decided that code wouldn't work anymore. Strange...

@Y-Less
Copy link
Member

Y-Less commented Apr 26, 2017

Well if some other code used the variable and that changed, then that could trigger the bug in other places, since they would now be the only places the variable was used. Glad it helped though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants