View Issue Details

IDProjectCategoryView StatusLast Update
0020827FPCCompilerpublic2011-12-11 18:02
ReporterKornel Kisielewicz Assigned ToSergei Gorelkin  
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
PlatformWindowsOSWindows 
Product Version2.4.4 
Fixed in Version3.0.0 
Summary0020827: Wrong code generated for custom enumerators
DescriptionSometimes the compiler generates wrong field addresses for loop object when using a custom enumerator over a container class.
Steps To ReproduceRun the attached program. The program should return Success, returns Failure.
Additional InformationI'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.
TagsNo tags attached.
Fixed in Revision19668
FPCOldBugId
FPCTarget
Attached Files

Relationships

has duplicate 0023641 resolvedSergei Gorelkin Compiler produces wrong code when enumarator is used in O2 and O3 

Activities

2011-12-06 23:58

 

enumerator_bug.pas (1,931 bytes)

Sergei Gorelkin

2011-12-08 15:52

developer   ~0054830

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.

Kornel Kisielewicz

2011-12-08 20:57

reporter   ~0054839

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.
enumerator_bug_2 (1,931 bytes)   

Kornel Kisielewicz

2011-12-08 21:02

reporter   ~0054840

Attached is the corrected version. Returns "Failure" for me also.

Sergei Gorelkin

2011-12-08 21:40

developer   ~0054841

The second file is exactly identical to the first one.

Kornel Kisielewicz

2011-12-08 21:55

reporter   ~0054842

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>

Kornel Kisielewicz

2011-12-08 21:56

reporter   ~0054843

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 :(

Sergei Gorelkin

2011-12-08 22:10

developer   ~0054845

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.

Kornel Kisielewicz

2011-12-08 22:53

reporter   ~0054847

Last edited: 2011-12-08 22:54

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.

Sergei Gorelkin

2011-12-08 23:37

developer   ~0054850

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.

Kornel Kisielewicz

2011-12-11 03:12

reporter   ~0054923

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.

Sergei Gorelkin

2011-12-11 18:00

developer   ~0054953

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.

Issue History

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