View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0020827 | FPC | Compiler | public | 2011-12-06 23:58 | 2011-12-11 18:02 |
Reporter | Kornel Kisielewicz | Assigned To | Sergei Gorelkin | ||
Priority | normal | Severity | major | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Platform | Windows | OS | Windows | ||
Product Version | 2.4.4 | ||||
Fixed in Version | 3.0.0 | ||||
Summary | 0020827: Wrong code generated for custom enumerators | ||||
Description | Sometimes the compiler generates wrong field addresses for loop object when using a custom enumerator over a container class. | ||||
Steps To Reproduce | Run the attached program. The program should return Success, returns Failure. | ||||
Additional Information | I've hit a bug with a custom enumerator over a node tree. I tried minimizing and extracting the faulty code. Attached is a program that simulates that error. The single node (in the original there were many) in the container has some fields -- iterating through them with our custom iterator doesn't properly work, because in the loop the addresses are wrongly associated for the fields. Also reproduced on 2.6.0 rc1 and Linux. | ||||
Tags | No tags attached. | ||||
Fixed in Revision | 19668 | ||||
FPCOldBugId | |||||
FPCTarget | |||||
Attached Files |
|
has duplicate | 0023641 | resolved | Sergei Gorelkin | Compiler produces wrong code when enumarator is used in O2 and O3 |
2011-12-06 23:58
|
|
|
Enumerator's MoveNext method should return True to continue enumeration, False to exit the loop. In the attached example, the logic of MoveNext is inverse. It returns False on the first call, consequently the loop is never executed. Swapping True and False makes the example report success. |
|
That was a typo when minimizing the code (the original had it properly and works in some cases correctly). Even with the true/false switched, I still get Failure. |
2011-12-08 21:01
|
enumerator_bug_2 (1,931 bytes)
{$MODE OBJFPC} {$INTERFACES CORBA} {$COPERATORS ON} {$INLINE ON} {$OPTIMIZATION ON} type TNode = class ID : byte; MyType : byte; Amount : word; constructor Create(); function IsOK : boolean; end; type TContainer = class; type TContainerEnumerator = class Contents : TNode; IsDone : boolean; constructor Create(Parent : TContainer); function MoveNext : Boolean; function GetCurrent : TNode; property Current : TNode read GetCurrent; end; type TContainer = class Contents : TNode; constructor Create(node : TNode); function Seek(SeekID : byte) : TNode; function GetEnumerator : TContainerEnumerator; end; constructor TNode.Create(); begin ID := 20; MyType := 5; Amount := 24; end; function TNode.IsOK : boolean; begin Exit(MyType = 5); end; constructor TContainerEnumerator.Create(Parent : TContainer); begin Contents := Parent.Contents; IsDone := false; end; function TContainerEnumerator.MoveNext : Boolean; begin if IsDone then Exit(false); IsDone := true; Exit(true); end; function TContainerEnumerator.GetCurrent : TNode; begin Exit(Contents); end; constructor TContainer.Create(node : TNode); begin Contents := node; end; function TContainer.Seek(SeekID : byte) : TNode; var node : TNode; amount : word; begin Seek := nil; amount := 255; for node in Self do if node.IsOK then if node.ID = SeekID then if node.Amount < amount then begin Seek := node; amount := node.Amount; end; end; function TContainer.GetEnumerator : TContainerEnumerator; begin Exit(TContainerEnumerator.Create(Self)) end; var node : TNode; container : TContainer; begin node := TNode.Create(); container := TContainer.Create(node); if container.Seek(20) = nil then writeln('Failure') else writeln('Success'); end. |
|
Attached is the corrected version. Returns "Failure" for me also. |
|
The second file is exactly identical to the first one. |
|
d:\Temp\tt>diff enumerator_bug.pas enumerator_bug_2 54c54 < if IsDone then Exit(true); --- > if IsDone then Exit(false); 56c56 < Exit(false); --- > Exit(true); d:\Temp\tt> |
|
d:\Temp\tt>fpc enumerator_bug.pas Free Pascal Compiler version 2.4.4 [2011/11/04] for i386 Copyright (c) 1993-2010 by Florian Klaempfl Target OS: Win32 for i386 Compiling enumerator_bug.pas Linking enumerator_bug.exe 98 lines compiled, 0.0 sec , 29328 bytes code, 2264 bytes data d:\Temp\tt>fpc enumerator_bug_2 Free Pascal Compiler version 2.4.4 [2011/11/04] for i386 Copyright (c) 1993-2010 by Florian Klaempfl Target OS: Win32 for i386 Compiling enumerator_bug_2 Linking enumerator_bug_2.exe 98 lines compiled, 0.0 sec , 29328 bytes code, 2264 bytes data d:\Temp\tt>enumerator_bug.exe Failure d:\Temp\tt>enumerator_bug_2.exe Failure d:\Temp\tt> I don't know how much more clear I can make this :( |
|
Oh, sorry. I was comparing enumerator_bug_2 with the locally fixed version of enumerator_bug.pas, not with the original one. No surprise they were identical. But still, testing the fixed variant with FPC trunk prints 'success' for me in win32. |
|
I've had at least two people have the double failure that I've experienced. Both were on Windows 7 64-bit, using 32-bit compiler. I've tried on linux, and indeed the corrected version works properly. When hunting for the problem in the original code (I can paste the relevant fragment if needed) my friend noted that the variables on the stack are not referenced correctly : http://pastebin.com/EHRS75n6 I'll try testing it on trunk tommorow, but as noted, 2.6.0rc1 still has double Failure for me. |
|
Now finally I understand... I couldn't reproduce the issue with trunk because I already fixed the code generation in r19668 (code optimization was not inhibited when creating a try..finally block internally). The issue could only appear on i386-win32 and i386-linux targets. Please test trunk after r19668, it should work correctly. |
|
Success :). Yes indeed in trunk it's fixed. Note and a big plea -- I've noticed that it's *not* fixed in neither 2.6.0rc1, nor 2.5.1 -- could you make sure the fix will make it's way into 2.6.0 final? Any use of custom iterators is dangerous in this situation, and prevents me from using it in production code. |
|
Unfortunately it is too late for 2.6.0, given that the fix was made without direct encountering the bug and may have not yet known side effects. The wrong code is generated only with level 2 optimization (more precisely, with stackframe optimization which is part of level 2). Using enumerators without optimization or with level 1 optimization remains safe. |
Date Modified | Username | Field | Change |
---|---|---|---|
2011-12-06 23:58 | Kornel Kisielewicz | New Issue | |
2011-12-06 23:58 | Kornel Kisielewicz | File Added: enumerator_bug.pas | |
2011-12-08 15:52 | Sergei Gorelkin | Note Added: 0054830 | |
2011-12-08 15:53 | Sergei Gorelkin | Status | new => resolved |
2011-12-08 15:53 | Sergei Gorelkin | Resolution | open => no change required |
2011-12-08 15:53 | Sergei Gorelkin | Assigned To | => Sergei Gorelkin |
2011-12-08 20:57 | Kornel Kisielewicz | Status | resolved => feedback |
2011-12-08 20:57 | Kornel Kisielewicz | Resolution | no change required => reopened |
2011-12-08 20:57 | Kornel Kisielewicz | Note Added: 0054839 | |
2011-12-08 21:01 | Kornel Kisielewicz | File Added: enumerator_bug_2 | |
2011-12-08 21:02 | Kornel Kisielewicz | Note Added: 0054840 | |
2011-12-08 21:40 | Sergei Gorelkin | Note Added: 0054841 | |
2011-12-08 21:55 | Kornel Kisielewicz | Note Added: 0054842 | |
2011-12-08 21:56 | Kornel Kisielewicz | Note Added: 0054843 | |
2011-12-08 22:10 | Sergei Gorelkin | Note Added: 0054845 | |
2011-12-08 22:53 | Kornel Kisielewicz | Note Added: 0054847 | |
2011-12-08 22:54 | Kornel Kisielewicz | Note Edited: 0054847 | |
2011-12-08 22:54 | Kornel Kisielewicz | Note Edited: 0054847 | |
2011-12-08 23:37 | Sergei Gorelkin | Note Added: 0054850 | |
2011-12-11 03:12 | Kornel Kisielewicz | Note Added: 0054923 | |
2011-12-11 18:00 | Sergei Gorelkin | Note Added: 0054953 | |
2011-12-11 18:02 | Sergei Gorelkin | Fixed in Revision | => 19668 |
2011-12-11 18:02 | Sergei Gorelkin | Status | feedback => resolved |
2011-12-11 18:02 | Sergei Gorelkin | Fixed in Version | => 2.7.1 |
2011-12-11 18:02 | Sergei Gorelkin | Resolution | reopened => fixed |
2013-01-13 18:05 | Sergei Gorelkin | Relationship added | has duplicate 0023641 |