View Issue Details

IDProjectCategoryView StatusLast Update
0037780FPCCompilerpublic2020-09-20 20:09
ReporterMichal Gawrycki Assigned ToJonas Maebe  
PrioritynormalSeverityminorReproducibilitysometimes
Status closedResolutionfixed 
Platformi386OSWin32 
Product Version3.3.1 
Fixed in Version3.3.1 
Summary0037780: Wrong code generation with optimization level 2 and above (i386 win32)
DescriptionInvalid 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.
TagsNo tags attached.
Fixed in Revision46905
FPCOldBugId
FPCTarget-
Attached Files

Relationships

related to 0037714 closedMarco van de Voort importtl fails with EAccessViolation exception 

Activities

Michal Gawrycki

2020-09-20 13:47

reporter  

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.

testbug.lpr (282 bytes)   

Michal Gawrycki

2020-09-20 13:48

reporter   ~0125661

This issue is related to 0037714

jamie philbrook

2020-09-20 14:34

reporter   ~0125664

yes it does look strange however, I think a closer look is needed because at first glance I don't see an issue.

Jonas Maebe

2020-09-20 14:44

manager   ~0125665

Thanks for the test program.

jamie philbrook

2020-09-20 15:08

reporter   ~0125668

Last edited: 2020-09-20 15:10

View 2 revisions

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...

Jonas Maebe

2020-09-20 15:14

manager   ~0125669

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.

jamie philbrook

2020-09-20 15:45

reporter   ~0125673

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.

Jonas Maebe

2020-09-20 15:52

manager   ~0125674

> 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.

Michal Gawrycki

2020-09-20 20:09

reporter   ~0125680

Works correctly. Thanks for fast reaction.
Closed.

Issue History

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