[fpc-devel] An optimizer bug?

[Sergei Gorelkin]

Sergei Gorelkin wrote:

I had isolated the issue, it is maybechangetemp() in ncgld.pas. If I 
modify this function to always return false, the issue is gone. Actually 
the failure is caused by incomplete deallocation of the AnsiString temp 
variable - its initialization and finalization code is not deleted.

...But what the hell is happening? First, at pass1 compiler does a whole 
load of checks, decides that the assignment of function result cannot be 
optimized, and allocates an intermediate temp. Then, at pass2 (and with 
-O2 specified), it optimizes this assignment and tries to delete the 
temp, based solely on the fact that the right side of assignment is 
LOC_REFERENCE. All escape analysis from pass1 is void then?

How should this be fixed? Guess I can add a check for Ansi/WideString 
resultdef before call to maybechangetemp(), but it looks very like 
spaghetti to me...

Sergei

> It looks like my experiments with returning strings in parameters 
> introduced (or, more likely, triggered an existing) bug.
> 
> First, attached is the patch itself. It is pretty trivial, and it works 
> fine with smaller programs. Compiling the compiler itself also works, 
> but only if -O2 is not specified. With -O2 it starts crashing.
> 
> Digging in shows that the faulting function is cfileutl.PathExists(), 
> and its assembler code looks this:
> 
> // no optimization, code is correct
> CFILEUTL_PATHEXISTS$ANSISTRING$BOOLEAN$$BOOLEAN:
>         push    ebp
>         mov    ebp,esp
>         sub    esp,24
>         mov    dword ptr [ebp-4],eax
>         mov    byte ptr [ebp-8],dl
> ; Initializing two temps
>         mov    dword ptr [ebp-20],0
>         mov    dword ptr [ebp-24],0
>         cmp    dword ptr [ebp-4],0
>         je    @@j954
>         jmp    @@j955
> 
> // with -O2
> CFILEUTL_PATHEXISTS$ANSISTRING$BOOLEAN$$BOOLEAN:
>         push    ebp
>         mov    ebp,esp
>         sub    esp,20
>         mov    dword ptr [ebp-20],ebx
>         mov    dword ptr [ebp-4],eax
>         mov    bl,dl
> ; Bug here. The following two instructions should NOT have the same 
> offsets! One of the temps is left uninitialized.
>         mov    dword ptr [ebp-12],0
>         mov    dword ptr [ebp-12],0
>         test    eax,eax
>         jne    @@j955
> 
> With unpatched compiler, there is only one temp in this function, so the 
> described behaviour cannot be observed.
> I think it's an existing bug because unoptimized code is correct, and 
> the optimizer should never disrupt its correctness.
> Anyway, I'm a bit lost at this point and need at least a hint where to 
> look.