View Issue Details

IDProjectCategoryView StatusLast Update
0034385FPCCompilerpublic2018-11-18 11:11
Reporternoname012Assigned ToJonas Maebe 
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
PlatformOSWindowsOS Version10
Product Version3.3.1Product Build 
Target VersionFixed in Version3.3.1 
Summary0034385: Error in range test
DescriptionI tested a modified version of fpc\trunk\tests\tbs\tb0652.pp:

program rangeTest;
const
  w : dword = 123;
  n : dword = 48;
begin
  if (w<=1) and (w>=10) then
    writeln('error 1-10');
  if (w>=1) and (w<=1000) then
    writeln('ok')
  else
    writeln('error 1-1000');
  if (n>44)and(n<48) then
    writeln('error 48');
end.

The result is:

E:\lazarus\fpc\bin\i386-win32>fpc E:\rangeTest\rangeTest.pp -O3
Free Pascal Compiler version 3.3.1 [2018/10/02] for i386
Copyright (c) 1993-2018 by Florian Klaempfl and others
Target OS: Win32 for i386
Compiling E:\rangeTest\rangeTest.pp
Linking E:\rangeTest\rangeTest.exe
15 lines compiled, 0.1 sec, 26896 bytes code, 1300 bytes data

E:\lazarus\fpc\bin\i386-win32>E:\rangeTest\rangeTest.exe
ok
error 48


TagsNo tags attached.
Fixed in Revision40344
FPCOldBugId
FPCTarget
Attached Files
  • rangeTest.pp (281 bytes)
    program rangeTest;
    const
      w : dword = 123;
      n : dword = 48;
    begin
      if (w<=1) and (w>=10) then
        writeln('error 1-10');
      if (w>=1) and (w<=1000) then
        writeln('ok')
      else
        writeln('error 1-1000');
      if (n>44)and(n<48) then
        writeln('error 48');
    end.
    
    
    rangeTest.pp (281 bytes)
  • rangetst.pas (3,119 bytes)
    { https://bugs.freepascal.org/view.php?id=34385 }
    program rangeTst;
    
    {$optimization on} { level 2 }
    
    const  _44 = 49;  { 0..47, 48, 49, 50 - worth to try }
           _48 = 48;
    
    var
      n : dword ;
    
    {test range defined by const _44 and _48 }
    function range2(n : dword):boolean;
    var b1, err : boolean;
    var x, y : dword;
    
    begin
         b1:=false;
         err:=false;
         x:=_44;
         y:=_48;
    
         {test: N is in range or isn't}
         {make sure - no optimization taken here}
         if n>x then
              if n<y then
                   b1:=true;
    
         { test  in range with constant boundaries can be optimized by compiler}
         if (n>_44)and(n<_48) then
         begin
            if not b1 then begin err:=true; writeln('error < > ',n); end;
         end
            else if b1 then begin err:=true;  writeln('error < > ',n);  end;
    
        if (n>=_44+1)and(n<=_48-1) then
        begin
            if not b1 then  begin err:=true; writeln('error <= >= ',n); end;
        end
            else if b1 then begin err:=true; writeln('error <= >= ',n); end;
    
        if (n>_44)and(n<=_48-1) then
        begin
              if not b1 then begin err:=true; writeln('error < >= ',n); end;
        end
            else if b1 then begin err:=true; writeln('error < >= ',n); end;
    
    
        if (n>=_44+1)and(n<_48) then
        begin
              if not b1 then begin err:=true;  writeln('error <= > ',n); end;
        end
            else if b1 then begin err:=true; writeln('error <= > ',n); end;
    
        range2:=err;
    end;
    
    {test fiksed range  45..47 }
    function range(n : dword):boolean;
    var b1, err : boolean;
    var x, y : dword;
    
    begin
         b1:=false;
         err:=false;
         x:=44;
         y:=48;
    
         {test: N is in range or isn't}
         {no optimization taken here}
         if n>x then
              if n<y then
                   b1:=true;
    
         { test in range with constant boundaries can be optimized by compiler}
         if (n>44)and(n<48) then
         begin
            if not b1 then begin err:=true; writeln('error < > ',n); end;
         end
            else if b1 then begin err:=true;  writeln('error < > ',n);  end;
    
        if (n>=45)and(n<=47) then
        begin
            if not b1 then  begin err:=true; writeln('error <= >= ',n); end;
        end
            else if b1 then begin err:=true; writeln('error <= >= ',n); end;
    
        if (n>44)and(n<=47) then
        begin
              if not b1 then begin err:=true; writeln('error < >= ',n); end;
        end
            else if b1 then begin err:=true; writeln('error < >= ',n); end;
    
    
        if (n>=45)and(n<48) then
        begin
              if not b1 then begin err:=true;  writeln('error <= > ',n); end;
        end
            else if b1 then begin err:=true; writeln('error <= > ',n); end;
    
        range:=err;
    end;
    
    var err, err1 : boolean;
    
    begin
    
         err:=false;
    
         writeln('1. test:  44 < N < 48 ');
         for n:=0 to 100 do
         begin
              err1:= range (n);
              err:= err or err1;
         end;
    
    
         if _44 > _48 then
              writeln('2. test: (',(_44),' < N)  and ( N <',_48,') ')
         else
              writeln('2. test:  ',_44,' < N < ',_48,' ');
    
         for n:=0 to 100 do
         begin
              err1:= range2 (n);
              err:= err or err1;
         end;
    
    
         if err then
              halt(1)
         else writeln('Ok');
    
    end.
    
    rangetst.pas (3,119 bytes)
  • nadd_range_fix.patch (1,018 bytes)
    Index: compiler/nadd.pas
    ===================================================================
    --- compiler/nadd.pas	(revision 40002)
    +++ compiler/nadd.pas	(working copy)
    @@ -377,7 +377,7 @@
     
         function taddnode.simplify(forinline : boolean) : tnode;
     
    -      function is_range_test(nodel, noder: taddnode; var value: tnode; var cl,cr: Tconstexprint): boolean;
    +      function is_range_test(nodel, noder: taddnode; var value: tnode; var cl,cr: {$if defined(POWERPC) or defined(POWERPC64)}  Tconstexprint {$else} qword {$endif}): boolean;
             const
               is_upper_test: array[ltn..gten] of boolean = (true,true,false,false);
               inclusive_adjust: array[boolean,ltn..gten] of integer = ((1,0,-1,0),
    @@ -460,7 +460,7 @@
             resultset : Tconstset;
             res,
             b       : boolean;
    -        cr, cl  : Tconstexprint;
    +        cr, cl  : {$if defined(POWERPC) or defined(POWERPC64)}  Tconstexprint {$else} qword {$endif};
           begin
             result:=nil;
             l1:=0;
    nadd_range_fix.patch (1,018 bytes)

Relationships

related to 0034565 resolvedFlorian The bug in nadd.pas, last detailed in issue 0034385 which was today marked as "fixed", is not in fact in any way fixed. 

Activities

noname012

2018-10-04 21:41

reporter  

rangeTest.pp (281 bytes)
program rangeTest;
const
  w : dword = 123;
  n : dword = 48;
begin
  if (w<=1) and (w>=10) then
    writeln('error 1-10');
  if (w>=1) and (w<=1000) then
    writeln('ok')
  else
    writeln('error 1-1000');
  if (n>44)and(n<48) then
    writeln('error 48');
end.

rangeTest.pp (281 bytes)

noname012

2018-10-18 20:59

reporter   ~0111459

It is related with optimization that transform unsigned comparisons of (v>=x) and (v<=y) into (v-x)<(y-x).

Cyrax

2018-10-18 21:10

reporter   ~0111460

The related bug report : https://bugs.freepascal.org/view.php?id=34292

noname012

2018-10-19 07:35

reporter   ~0111470

Tested with r39986 and the error is still present

Marģers

2018-10-19 21:29

reporter  

rangetst.pas (3,119 bytes)
{ https://bugs.freepascal.org/view.php?id=34385 }
program rangeTst;

{$optimization on} { level 2 }

const  _44 = 49;  { 0..47, 48, 49, 50 - worth to try }
       _48 = 48;

var
  n : dword ;

{test range defined by const _44 and _48 }
function range2(n : dword):boolean;
var b1, err : boolean;
var x, y : dword;

begin
     b1:=false;
     err:=false;
     x:=_44;
     y:=_48;

     {test: N is in range or isn't}
     {make sure - no optimization taken here}
     if n>x then
          if n<y then
               b1:=true;

     { test  in range with constant boundaries can be optimized by compiler}
     if (n>_44)and(n<_48) then
     begin
        if not b1 then begin err:=true; writeln('error < > ',n); end;
     end
        else if b1 then begin err:=true;  writeln('error < > ',n);  end;

    if (n>=_44+1)and(n<=_48-1) then
    begin
        if not b1 then  begin err:=true; writeln('error <= >= ',n); end;
    end
        else if b1 then begin err:=true; writeln('error <= >= ',n); end;

    if (n>_44)and(n<=_48-1) then
    begin
          if not b1 then begin err:=true; writeln('error < >= ',n); end;
    end
        else if b1 then begin err:=true; writeln('error < >= ',n); end;


    if (n>=_44+1)and(n<_48) then
    begin
          if not b1 then begin err:=true;  writeln('error <= > ',n); end;
    end
        else if b1 then begin err:=true; writeln('error <= > ',n); end;

    range2:=err;
end;

{test fiksed range  45..47 }
function range(n : dword):boolean;
var b1, err : boolean;
var x, y : dword;

begin
     b1:=false;
     err:=false;
     x:=44;
     y:=48;

     {test: N is in range or isn't}
     {no optimization taken here}
     if n>x then
          if n<y then
               b1:=true;

     { test in range with constant boundaries can be optimized by compiler}
     if (n>44)and(n<48) then
     begin
        if not b1 then begin err:=true; writeln('error < > ',n); end;
     end
        else if b1 then begin err:=true;  writeln('error < > ',n);  end;

    if (n>=45)and(n<=47) then
    begin
        if not b1 then  begin err:=true; writeln('error <= >= ',n); end;
    end
        else if b1 then begin err:=true; writeln('error <= >= ',n); end;

    if (n>44)and(n<=47) then
    begin
          if not b1 then begin err:=true; writeln('error < >= ',n); end;
    end
        else if b1 then begin err:=true; writeln('error < >= ',n); end;


    if (n>=45)and(n<48) then
    begin
          if not b1 then begin err:=true;  writeln('error <= > ',n); end;
    end
        else if b1 then begin err:=true; writeln('error <= > ',n); end;

    range:=err;
end;

var err, err1 : boolean;

begin

     err:=false;

     writeln('1. test:  44 < N < 48 ');
     for n:=0 to 100 do
     begin
          err1:= range (n);
          err:= err or err1;
     end;


     if _44 > _48 then
          writeln('2. test: (',(_44),' < N)  and ( N <',_48,') ')
     else
          writeln('2. test:  ',_44,' < N < ',_48,' ');

     for n:=0 to 100 do
     begin
          err1:= range2 (n);
          err:= err or err1;
     end;


     if err then
          halt(1)
     else writeln('Ok');

end.
rangetst.pas (3,119 bytes)

Marģers

2018-10-19 21:37

reporter   ~0111476

Last edited: 2018-10-19 21:44

View 3 revisions

problem are with optimization of (v > x) and (v < y); (v > x) and (v <= y); (v >= x) and (v < y)

possible solution: convert (v > x) and (v < y) into (v >= x+1) and (v <= y-1) and then apply working optimization of (v>=z) and (v<=q)

added test program that show problem cases

Do-wan Kim

2018-10-21 03:57

reporter  

nadd_range_fix.patch (1,018 bytes)
Index: compiler/nadd.pas
===================================================================
--- compiler/nadd.pas	(revision 40002)
+++ compiler/nadd.pas	(working copy)
@@ -377,7 +377,7 @@
 
     function taddnode.simplify(forinline : boolean) : tnode;
 
-      function is_range_test(nodel, noder: taddnode; var value: tnode; var cl,cr: Tconstexprint): boolean;
+      function is_range_test(nodel, noder: taddnode; var value: tnode; var cl,cr: {$if defined(POWERPC) or defined(POWERPC64)}  Tconstexprint {$else} qword {$endif}): boolean;
         const
           is_upper_test: array[ltn..gten] of boolean = (true,true,false,false);
           inclusive_adjust: array[boolean,ltn..gten] of integer = ((1,0,-1,0),
@@ -460,7 +460,7 @@
         resultset : Tconstset;
         res,
         b       : boolean;
-        cr, cl  : Tconstexprint;
+        cr, cl  : {$if defined(POWERPC) or defined(POWERPC64)}  Tconstexprint {$else} qword {$endif};
       begin
         result:=nil;
         l1:=0;
nadd_range_fix.patch (1,018 bytes)

Do-wan Kim

2018-10-21 03:59

reporter   ~0111493

r39990 breaks launch lazarus IDE again.

Julian Puhl

2018-11-13 13:03

reporter   ~0111948

I tried to compile Lazarus with current trunk (r40300) and it still breaks it (Lazarus wants to upgrade config even though it is not necessary and then it crashes). I had to comment this optimization out by hand (line 1061-1075) and now it works again. I don't understand why this is left in trunk when it breaks code? Shouldn't tests be catching it?

Thaddy de Koning

2018-11-13 14:54

reporter   ~0111949

Last edited: 2018-11-13 14:54

View 2 revisions

I have no problems with it on arm with -O2. It simply compiles and prints OK.
With -O4 it errors indeed. {$R+} does not matter, which should have caught any range errors here. Both todays trunk.

Julian Puhl

2018-11-13 15:19

reporter   ~0111952

Have you tried building Lazarus with -O2? I am on Win32. Maybe it only happens there? I guess we'll have to find the code from Lazarus which breaks with this optimization.

Thaddy de Koning

2018-11-13 17:34

reporter   ~0111954

Last edited: 2018-11-13 17:36

View 3 revisions

Maybe you miss-read the above. I am confirming. This is an FPC bug related to optimization settings.

Julian Puhl

2018-11-13 18:28

reporter   ~0111955

Yes, but you said it works with -O2. This is not the case with Lazarus, so it must be a different bug.

Thaddy de Koning

2018-11-13 22:38

reporter   ~0111958

A standard Lazarus is compiled with -O3 you should know that.

Julian Puhl

2018-11-13 22:47

reporter   ~0111959

My profile for optimized IDE says -O2 and that's what I asked. Maybe its old. But you want to test the lowest optimization level which is possible anyway to eliminate side effects from other optimizations.

Thaddy de Koning

2018-11-14 17:10

reporter   ~0111973

Julian,

It is a bug - never mind Lazarus for now - that I can confirm on multiple platforms - of optimization levels be proven to break code. You did good to report it here.
The simple example is better for determination than a full Lazarus project.
If it later appears that Lazarus is still broken, we test some more ;)

Julian Puhl

2018-11-14 19:06

reporter   ~0111976

I think we are talking about different things here. To make it simple I compiled the test program posted above with x64 and x86 build of FPC. In both cases it fails with -O2. Since this is the level were the optimization is enabled (you can see it in the source of nadd.pas) and it works when I comment it out, it is safe to assume that this optimization breaks the code.

Since you posted it worked for you with -O2 I assumed the code is not triggering the bug. This is the reason I wanted to find the code in Lazarus so that another simple example could be generated. But now I know the above example triggers the bug for x64 and x86. Since for your CPU target it only gets triggered with -O4, it makes it more difficult to deduce anything. The optimization is active for you with -O2, but yet the results are correct. This means you can not safely make the assumption that the code gets broken for you with this optimization. The bug with -O4 might just be a side effect of any optimization applied afterwards. It is impossible to tell without further testing.

I hope I made myself clear now :)

Akira1364

2018-11-15 00:12

reporter   ~0111982

Last edited: 2018-11-15 03:09

View 5 revisions

It's definitely that QWord vs Tconstexprint change that specifically causes the breakage. It simply has to be QWord (at least on x86 architecture CPUs) or the compiler ends up generating invalid code.

More broadly I'm pretty sure it's related to the fact that FPC just generally seems to have difficulty properly distinguishing between Int64 and QWord in all cases, meaning it's probably getting confused at some point by the Tconstexprint assignment operator overloads...

Thaddy de Koning

2018-11-16 14:59

reporter   ~0112001

Yes there may be two different things:
- the simple code included by Julian fails on higher optimization settings than -O2 I can confirm this on arm, aarch and x86_win64.
- there may be a related but other bug that causes Lazarus to crash. (As per Akira) There are a lot of places where 64 bit values are interpreted in a legacy way, specifically in the Lazarus code case.
As it stands, you don't need Lazarus to reproduce the bug. (My tests were compiled from a prompt fpc -CX -XXs -Ox where x is the optimization level)

Issue History

Date Modified Username Field Change
2018-10-04 21:41 noname012 New Issue
2018-10-04 21:41 noname012 File Added: rangeTest.pp
2018-10-18 20:59 noname012 Note Added: 0111459
2018-10-18 21:10 Cyrax Note Added: 0111460
2018-10-19 07:35 noname012 Note Added: 0111470
2018-10-19 21:29 Marģers File Added: rangetst.pas
2018-10-19 21:37 Marģers Note Added: 0111476
2018-10-19 21:43 Marģers Note Edited: 0111476 View Revisions
2018-10-19 21:44 Marģers Note Edited: 0111476 View Revisions
2018-10-21 03:57 Do-wan Kim File Added: nadd_range_fix.patch
2018-10-21 03:59 Do-wan Kim Note Added: 0111493
2018-11-13 13:03 Julian Puhl Note Added: 0111948
2018-11-13 14:54 Thaddy de Koning Note Added: 0111949
2018-11-13 14:54 Thaddy de Koning Note Edited: 0111949 View Revisions
2018-11-13 15:19 Julian Puhl Note Added: 0111952
2018-11-13 17:34 Thaddy de Koning Note Added: 0111954
2018-11-13 17:35 Thaddy de Koning Note Edited: 0111954 View Revisions
2018-11-13 17:36 Thaddy de Koning Note Edited: 0111954 View Revisions
2018-11-13 18:28 Julian Puhl Note Added: 0111955
2018-11-13 22:38 Thaddy de Koning Note Added: 0111958
2018-11-13 22:47 Julian Puhl Note Added: 0111959
2018-11-14 17:10 Thaddy de Koning Note Added: 0111973
2018-11-14 19:06 Julian Puhl Note Added: 0111976
2018-11-15 00:12 Akira1364 Note Added: 0111982
2018-11-15 00:15 Akira1364 Note Edited: 0111982 View Revisions
2018-11-15 00:16 Akira1364 Note Edited: 0111982 View Revisions
2018-11-15 03:08 Akira1364 Note Edited: 0111982 View Revisions
2018-11-15 03:09 Akira1364 Note Edited: 0111982 View Revisions
2018-11-16 14:59 Thaddy de Koning Note Added: 0112001
2018-11-17 23:38 Jonas Maebe Fixed in Revision => 40344
2018-11-17 23:38 Jonas Maebe Status new => resolved
2018-11-17 23:38 Jonas Maebe Fixed in Version => 3.3.1
2018-11-17 23:38 Jonas Maebe Resolution open => fixed
2018-11-17 23:38 Jonas Maebe Assigned To => Jonas Maebe
2018-11-18 10:15 noname012 Status resolved => closed
2018-11-18 11:11 Jonas Maebe Relationship added related to 0034565