View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0037780 | FPC | Compiler | public | 2020-09-20 13:47 | 2020-09-20 20:09 |
| Reporter | Michal Gawrycki | Assigned To | Jonas Maebe | ||
| Priority | normal | Severity | minor | Reproducibility | sometimes |
| Status | closed | Resolution | fixed | ||
| Platform | i386 | OS | Win32 | ||
| Product Version | 3.3.1 | ||||
| Fixed in Version | 3.3.1 | ||||
| Summary | 0037780: Wrong code generation with optimization level 2 and above (i386 win32) | ||||
| Description | Invalid code is generated for the following expression: # [16] if (TR.Val = 10) or ((TR.Val = 5) and (TR.Next^.Val = 5)) then cmpl $10,%eax je .Lj3 movl U_$P$TESTBUG_$$_TR+4,%edx movl (%edx),%edx xorl $5,%edx xorl $5,%eax orl %eax,%edx jne .Lj5 Last two comparisons seem to be linked together. If I understand correctly, last comparison (TR.Next^.Val = 5) should only be performed if result of second comparison (TR.Val = 5) is true. In this case, the TR.Next field is null and causes an AV exception. | ||||
| Tags | No tags attached. | ||||
| Fixed in Revision | 46905 | ||||
| FPCOldBugId | |||||
| FPCTarget | - | ||||
| Attached Files |
| ||||
| related to | 0037714 | closed | Marco van de Voort | importtl fails with EAccessViolation exception |
|
|
testbug.lpr (282 bytes)
program testbug;
type
PTestRec = ^TTestRec;
TTestRec = record
Val: Integer;
Next: PTestRec;
end;
var
TR: TTestRec;
begin
TR.Val := 6;
TR.Next := nil;
if (TR.Val = 10) or ((TR.Val = 5) and (TR.Next^.Val = 5)) then
Writeln('OK');
end.
|
|
|
This issue is related to 0037714 |
|
|
yes it does look strange however, I think a closer look is needed because at first glance I don't see an issue. |
|
|
Thanks for the test program. |
|
|
But you are enclosing two statements into one which forces the compiler to create two values for a compare operation without testing if those values (pointer wise) are valid. Technically I would say this is correct because you have not tested for a NULL pointer , you assume the compiler should know this and that is a bad assumption. The compiler should not need to create extra code to test for your invalid pointers. The older compilers used slightly different logic, more of the "CMP" of single units. That's ok but it hides the fact that you are passing it invalid pointers. There should always be a check point in there to test the validity of the Pointer, the compiler should not need to do this for you since it would cause a lot of false triggers otherwise. if you look, it already bypasses that code of invalid pointer load if the first on the left is valid and this skips around all of that.. That's my opinion, really but... |
|
|
Short-circuit boolean evaluation means that the compiler must never evaluate the next term of a boolean expression if the previous one failed, or at the very least that that semantically it cannot be observed if the compiled did it (e.g. for optimisation reasons). It doesn't matter what the previous and next conditions are or how they are written. |
|
|
depending on side effects of a compiler (older compilers) is bad practice. The first initial test does generate proper code to jump around the following, so the short circuit test is working. its the fact a bad pointer is given to the test when the first step fails and moves onto the next for which is needs to prep two values to complete its test. the compiler should not need to worry about bad pointers. To me this Is common 101 coding. ALWAYS TEST POINTER TO ENSURE its valid before using it.. Oh well. |
|
|
> depending on side effects of a compiler (older compilers) is bad practice. It's not a side-effect, it explicitly defined how "short-circuit boolean" evaluation works ({$b-} switch, which is enabled by default). > The first initial test does generate proper code to jump around the following, so the short circuit test is working. Short-circuit boolean evaluation applies to each expression (i.e., also to "(TR.Val = 5) and (TR.Next^.Val = 5)"), not just to the "top" one. > To me this Is common 101 coding. ALWAYS TEST POINTER TO ENSURE its valid before using it.. It's irrelevant whether you test a pointer or another value that defines whether the pointer is valid. |
|
|
Works correctly. Thanks for fast reaction. Closed. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2020-09-20 13:47 | Michal Gawrycki | New Issue | |
| 2020-09-20 13:47 | Michal Gawrycki | File Added: testbug.lpr | |
| 2020-09-20 13:48 | Michal Gawrycki | Note Added: 0125661 | |
| 2020-09-20 14:34 | jamie philbrook | Note Added: 0125664 | |
| 2020-09-20 14:44 | Jonas Maebe | Assigned To | => Jonas Maebe |
| 2020-09-20 14:44 | Jonas Maebe | Status | new => resolved |
| 2020-09-20 14:44 | Jonas Maebe | Resolution | open => fixed |
| 2020-09-20 14:44 | Jonas Maebe | Fixed in Version | => 3.3.1 |
| 2020-09-20 14:44 | Jonas Maebe | Fixed in Revision | => 46905 |
| 2020-09-20 14:44 | Jonas Maebe | FPCTarget | => - |
| 2020-09-20 14:44 | Jonas Maebe | Note Added: 0125665 | |
| 2020-09-20 14:46 | Jonas Maebe | Relationship added | related to 0037714 |
| 2020-09-20 15:08 | jamie philbrook | Note Added: 0125668 | |
| 2020-09-20 15:10 | jamie philbrook | Note Edited: 0125668 | View Revisions |
| 2020-09-20 15:14 | Jonas Maebe | Note Added: 0125669 | |
| 2020-09-20 15:45 | jamie philbrook | Note Added: 0125673 | |
| 2020-09-20 15:52 | Jonas Maebe | Note Added: 0125674 | |
| 2020-09-20 20:09 | Michal Gawrycki | Status | resolved => closed |
| 2020-09-20 20:09 | Michal Gawrycki | Note Added: 0125680 |
