pawn-lang / compiler

Pawn compiler for SA-MP with bug fixes and new features - runs on Windows, Linux, macOS
Other
303 stars 70 forks source link

Two globals given the same address #666

Open Y-Less opened 3 years ago

Y-Less commented 3 years ago

Issue description:

YSI has two variables, I@ and J@. In the previous commit they were defined as exactly:

https://github.com/pawn-lang/YSI-Includes/blob/7b48572ef7b1544a0548496f5967b7bcaade2a8f/YSI_Core/y_core/y_shortvar.inc#L101

Yet, compiling the code below they are both given the address 24, which causes the timer to loop forever.

Minimal complete verifiable example (MCVE):

#pragma option -a

#include <a_samp>
#include <YSI_Coding\y_timers>

main()
{
    printf("defer");
    defer MyTimer();
}

timer MyTimer[1000]()
{
    print("This is my timer");
}

I know this isn't an MCVE. Sorry.

Workspace Information:

Y-Less commented 3 years ago

OK, it seems to come from stock variables only conditionally used in the second pass.

Y-Less commented 3 years ago

OK, an actual MCVE:

#pragma option -a

native printf(const str[], ...);

stock
    I@ = 0,
    J@ = 0;

main()
{
    #if defined PP_main
        PP_main();
    #endif
}

#if defined _ALS_main
    #undef main
#else
    #define _ALS_main
#endif

#define main( PP_main(

main()
{
    printf("%d, %d", I@, J@);
}
Y-Less commented 3 years ago

Even minimaler CVE:

stock i, j;

main()
{
    #if defined Func
        Func();
    #endif
}

Func()
{
    i = j;
}
Y-Less commented 3 years ago

Just realised, this issue is evil!

Daniel-Cortez commented 3 years ago

On the first pass the compiler analyzes the use of each variable. If a variable is unused, it's not given a distinct address (the address is kept the same as for the previous variable, or 0 in case there were no other variables before the unused one, as in the CVE above). This is how the compiler throws out unused stock variables, so they don't take up extra memory.

So what happens in the CVE above is that on the 1'st pass variables i and j are considered unused and aren't given a distinct address. Then on the second pass both variables get used inside of an #if defined Func block because defined Func gets evaluated to a value different from the previous pass - the compiler can't know about this ahead of time, so it causes an undefined behavior.

I think the only reasonable way to fix this is to force a reparse each time an #if defined <function> block gets evaluated to a value different from the previous pass. In fact, this might help to fix many other potential problems caused by this kind of undefined behavior (e.g. #607). At the same time, forcing an extra compilation pass may probably break advanced third-party code, namely some of the YSI includes (I'm not very well acknowledged with the code in YSI, so I can't tell for sure).

Should we take the risk, or would it be better to leave everything as is? @Y-Less

Y-Less commented 3 years ago

Yeah, just leave it. I fixed this in a different way. Basically, just ensure callback hooks are always secretly public.

Daniel-Cortez commented 3 years ago

@Y-Less If you don't mind, I'll still try to experiment with forcing an extra pass and maybe make a PR if it doesn't break any code (or if it doesn't break too much code, at the very least). The undefined behavior I described in the previous post is one of the fundamental problems that causes several different bugs (beside this one and #607 there are more related bugs I haven't documented yet), which is why I think it would be great to fix all of them at once, and forcing an extra pass seems to be the only reasonable way to do that.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity.