View Issue Details

IDProjectCategoryView StatusLast Update
0034140FPCCompilerpublic2018-08-17 21:45
Reporter440bxAssigned To 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionno change required 
PlatformOSWin 7OS Version64bit
Product Version3.0.4Product Build 
Target VersionFixed in Version 
Summary0034140: compiler removes code it wrongly believes to be unreachable
DescriptionThe compiler wrongly assumes that some code is unreachable and removes it. This causes the code to produce wrong results.
Steps To ReproduceIt seems the problem is associated with subranges and types dependent on previously defined subranges. The compiler seems to believe that the subrange cannot be violated and removes any code that ensures values are in the proper range.
Additional InformationThe compiler emits warning stating that the code is unreachable when it is apparent that such warnings are incorrect.

Attached is a test program - including an executable - that exhibits the problem.
TagsNo tags attached.
Fixed in Revision
FPCOldBugId
FPCTarget
Attached Files

Relationships

related to 0016006 closedJonas Maebe Dead code mistake 

Activities

440bx

2018-08-17 08:36

reporter  

RangeChecking.7z (56,151 bytes)

Marģers

2018-08-17 10:49

reporter   ~0110076

isn't this duplicate of 0016006

440bx

2018-08-17 16:52

reporter   ~0110077

In the bug report you referred to, the reason for the incorrect behavior was attributed to the typecast and, there is no typecast in the example I provided.

There are many ways a variable can end up with an out of bounds value at run time, the compiler should not remove the programmer written range checking code. Plus, the conclusion that the code is unreachable is incorrect.

Jonas Maebe

2018-08-17 17:53

manager   ~0110080

If you enable range checking, you will get a run time error at the time the conversion error occurs (from an integer with value 256 to TINDEXES).

Regardless of how you manage to get an invalid value into a variable (explicit typecast, implicit conversion, disk I/O, memory corruption, side channel attack, ...), the compiler will always assume that a variable of a given type will contain a value that is valid for this type whenever you read it. A variable that contains a value that is not valid for its type results in undefined behaviour, regardless of whether it's an integer that's out of bounds or an ansistring that points to an invalid memory location.

If you wish to perform a manual range check, you have to tell the compiler to ignore what it knows about the TINDEXES type by typecasting the value back to integer.

440bx

2018-08-17 18:27

reporter   ~0110083

Last edited: 2018-08-17 18:29

View 2 revisions

I doubt there is any sense in proving your argument to be "faulty" (to put it very kindly) but, since compatibility with Delphi seems to be of at least some importance, then:

The way FPC handles the example is INCOMPATIBLE with Delphi.

Delphi executes the test (as a correctly written compiler should) and returns the correct value.

Hopefully, the fact that FPC's behavior is incompatible with Delphi will be enough for someone to correct the code.

FPC's conclusion that, the if statement is unreachable is INCORRECT.

Jonas Maebe

2018-08-17 18:48

manager   ~0110084

The Delphi argument only matters if it is documented (which it may be). If it is an implementation detail, then we are back at the "undefined behaviour" point.

If you wish to read more about a related discussion, there is the thread that starts with http://lists.freepascal.org/pipermail/fpc-devel/2017-July/038009.html and that contains a lot of interesting arguments.

I still think it makes no sense that a high level compiler should not exploit the available type information to the fullest extent under certain conditions.

440bx

2018-08-17 19:51

reporter   ~0110085

I'm afraid that is not an implementation detail. Whenever a _variable_ is being tested, the compiler cannot simply _assume_ that the variable is within the bounds of the type. That is incorrect.

The problem isn't in typecasting, it is in the dead code determination algorithm. It _mistakenly_ concludes that the code is unreachable.

In addition to reaching an incorrect conclusion about the code being unreachable, the compiler allowed a variable declared as being an integer to be used as being of type TINDEXES (and there is no typecast telling it to do that.)

Michael Van Canneyt

2018-08-17 20:27

administrator   ~0110087

Last edited: 2018-08-17 20:28

View 2 revisions

I compiled your program in Delphi with range checking.

It errors out, the same as in FPC.
And it should because you are feeding an invalid value to GetMessage which you explicitly declare to accept values between 0..2 .

So, the range check error tells you your code contains a flaw.

What happens in case of invalid values (i.e. wrong program logic)
is undefined, if you switch off range checking.

Unlike Delphi, the FPC compiler at least warns you what is wrong with your code:
RangeChecking.dpr(103,8) Warning: Comparison might be always false due to range of constant and expression
RangeChecking.dpr(104,3) Warning: unreachable code

There are other cases where Delphi and FPC differ in case of wrong values being supplied. But for correct code/values, FPC and Delphi will behave the same.

For wrong code (as in your case), there simply is no way to guarantee that FPC and Delphi will always behave the same.

440bx

2018-08-17 21:11

reporter   ~0110088

Last edited: 2018-08-17 21:13

View 3 revisions

It's obvious that there is something wrong with the code, it's done on purpose, to ensure that the sanity check performed by the if I > 2 is working properly. In a more complex piece of code, the error rarely is as obvious.

The compiler cannot accept an integer as being the same as a type TINDEXES (without a typecast forcing it to accept it) and then _arbitrarily_ and cavalierly _assume_ that an integer has the same range constraints as a type TINDEXES.

If the compiler accepts an integer instead of a TINDEXES then it cannot make any assumptions about the range, much less, get rid of sanity code to ensure the index is in the valid range and return a value that exposes the error.

What the compiler is doing in that case is semantically wrong.

While I have you here, is there any interest in solving the problem mentioned in "0034098: wrong address/pointer being assigned to a pchar variable" ?

Florian

2018-08-17 21:39

administrator   ~0110090

The rationale behind the behaviour has been explained and it still holds. For further discussions the mailing lists can be used.

Jonas Maebe

2018-08-17 21:45

manager   ~0110091

Last edited: 2018-08-17 21:45

View 2 revisions

Types that are assignment-compatible (such as integer and TINDEXES) do not necessarily have the same range. If you do not wish to manually insert range checking code when these assignments happen, the compiler can do it for you if you enable range checking.

Once you have assigned an invalid value to a variable, anything else you do with variable is undefined. The reason is that after assigning an invalid value, that variable could contain any value.

Case in point, in the following program it will contain 0 and hence your program would still print the wrong result for the given input (256) even if the compiler did not optimise anything away:

type
  TINDEXES = 0..2;
  tr = bitpacked record
    i: tindexes;
  end;

var
  r: tr;
  i: integer;
begin
  i:=256;
  r.i:=i;
  writeln(r.i);
end.

The reason is that the compiler has reduced the number of bits reserved for the storage of "i" in the bitpacked record because the programmer has told the compiler that it will never contain any value outside the range 0..2. Again, enabling range checking will catch this error. Alternatively, insert manual range checking before trying to store a value in the target variable that it cannot hold/represent.

Issue History

Date Modified Username Field Change
2018-08-17 08:36 440bx New Issue
2018-08-17 08:36 440bx File Added: RangeChecking.7z
2018-08-17 10:49 Marģers Note Added: 0110076
2018-08-17 16:52 440bx Note Added: 0110077
2018-08-17 17:53 Jonas Maebe Note Added: 0110080
2018-08-17 17:53 Jonas Maebe Status new => resolved
2018-08-17 17:53 Jonas Maebe Resolution open => no change required
2018-08-17 17:53 Jonas Maebe Assigned To => Jonas Maebe
2018-08-17 17:53 Jonas Maebe Relationship added related to 0016006
2018-08-17 18:27 440bx Note Added: 0110083
2018-08-17 18:27 440bx Status resolved => feedback
2018-08-17 18:27 440bx Resolution no change required => reopened
2018-08-17 18:29 440bx Note Edited: 0110083 View Revisions
2018-08-17 18:31 Jonas Maebe Assigned To Jonas Maebe =>
2018-08-17 18:32 Jonas Maebe Status feedback => new
2018-08-17 18:48 Jonas Maebe Note Added: 0110084
2018-08-17 19:51 440bx Note Added: 0110085
2018-08-17 20:27 Michael Van Canneyt Note Added: 0110087
2018-08-17 20:27 Michael Van Canneyt Status new => resolved
2018-08-17 20:27 Michael Van Canneyt Resolution reopened => no change required
2018-08-17 20:27 Michael Van Canneyt Assigned To => Michael Van Canneyt
2018-08-17 20:28 Michael Van Canneyt Note Edited: 0110087 View Revisions
2018-08-17 21:11 440bx Note Added: 0110088
2018-08-17 21:11 440bx Status resolved => feedback
2018-08-17 21:11 440bx Resolution no change required => reopened
2018-08-17 21:11 440bx Note Edited: 0110088 View Revisions
2018-08-17 21:13 440bx Note Edited: 0110088 View Revisions
2018-08-17 21:39 Florian Note Added: 0110090
2018-08-17 21:39 Florian Status feedback => closed
2018-08-17 21:39 Florian Assigned To Michael Van Canneyt =>
2018-08-17 21:39 Florian Resolution reopened => no change required
2018-08-17 21:45 Jonas Maebe Note Added: 0110091
2018-08-17 21:45 Jonas Maebe Note Edited: 0110091 View Revisions