All Projects FPC fpcprojects fpprofilerfpGUI Lazarus Packages PatchesLazarus CCR Mantis pas2js

View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0030409FPCCompilerpublic2016-07-25 07:432018-10-04 20:31
ReporterJared Davison Assigned ToMichael Van Canneyt  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionno change required 
Product Version2.6.4 
Summary0030409: FPC delphi mode behaviour does not honour Delphi documented object lifetimes
DescriptionDelphi defines the time of destruction of a reference counted object as: "the object is automatically destroyed when the last reference to it goes out of scope" (See http://docwiki.embarcadero.com/RADStudio/Seattle/en/Interface_References)

Steps To Reproduce{
Call EnsureDelphiReferenceSemantics in the the following program to test the compiler behaviour matches Delphi in regards to when it destroys interfaced objects that have reached a reference count of 0. The program simply checks the order in which items are added to a string list to verify the behaviour is as expected.
}

unit CheckCompilerInterfaceRefCountSemantics;

interface

uses SysUtils;

type
  ECompilerInterfaceSemanticsFailure = class(Exception);

procedure EnsureDelphiReferenceSemantics;

implementation

uses
  Classes;

var
  _sl : TStringList;

type
  TAutoObj = class(TInterfacedObject, IInterface)
  public
    constructor Create;
    destructor Destroy; override;
    class function EnterAutoExit: IInterface;
  end;

procedure Log(s: string);
begin
  _sl.Add(s);
end;

constructor TAutoObj.Create;
begin
  Log('0');
end;

destructor TAutoObj.Destroy;
begin
  Log('2');
  inherited;
end;

class function TAutoObj.EnterAutoExit: IInterface;
begin
  Result := TAutoObj.Create() as IInterface;
end;

procedure DoEnterAutoExit;
begin
  TAutoObj.EnterAutoExit;
  Log('1');
end;

procedure VerifyResults;
begin
  if not ((_sl.Count = 3) and (_sl[0] = '0') and (_sl[1] = '1') and (_sl[2] = '2')) then
  begin
    raise ECompilerInterfaceSemanticsFailure.Create('Object Pascal compiler does not support Delphi interface semantics where temporary varaiables are released at the end of scope. Please patch or use a different compiler.');
  end;
end;

procedure EnsureDelphiReferenceSemantics;
begin
  _sl := TStringList.Create;
  try
    DoEnterAutoExit;
    VerifyResults;
  finally
    _sl.Free;
  end;
end;

end.
Additional InformationPlease refer to the attached patch "intf.patch".

    Improve Delphi compatibility for lifetime of reference interfaced objects.
    
    In Delphi, interface references are destroyed when last reference goes out of scope (Delphi emulation) (versus freepascal behaviour that is immediately when its no longer referenced. This Delphi behaviour is documented in Delphi "the object is automatically destroyed when the last reference to it goes out of scope. " http://docwiki.embarcadero.com/RADStudio/Seattle/en/Interface_References
    
    This is important where object destruction has side effects whose timing of execution is of significance. This documented Delphi feature pattern is used in Delphi code to cause actions to happen automatically at the end of a scope.
    
    With this patch Delphi mode compatibility turns this behaviour on, or it can be turned on via a compiler directive SCOPED_INTERFACE_DESTROY. (better names are welcome)

It would be helpful if the FPC documentation could be updated if this patch as accepted.

Also see:
http://www.freepascal.org/docs-html/ref/refse47.html

The chosen answer in: http://stackoverflow.com/questions/9592654/what-are-the-differences-between-implementation-of-interfaces-in-delphi-and-laza

TagsNo tags attached.
Fixed in Revision
FPCOldBugId
FPCTarget
Attached Files

Relationships

related to 0026602 resolvedJonas Maebe Serious bug in TInterfacedObject / IUnknown 
related to 0025990 resolvedMichael Van Canneyt "as"-operator on interfaces references temporary variable 
related to 0009472 closedYuriy Sydorov "as" increase .RefCount in INTERFACE 
related to 0006035 closedJonas Maebe AV by interface access 

Activities

Jared Davison

2016-07-25 07:43

reporter  

intf.patch (4,217 bytes)    
commit ff467535ec8fb54ef867c32a0f75cdc1fe1bee45
Author: Jared Davison <jared@medical-objects.com.au>
Date:   Mon Jul 25 14:48:24 2016 +1000

    Improve Delphi compatibility for lifetime of reference interfaced objects.
    
    In Delphi, interface references are destroyed when last reference goes out of scope (Delphi emulation) (versus freepascal behaviour that is immediately when its no longer referenced. This Delphi behaviour is documented in Delphi "the object is automatically destroyed when the last reference to it goes out of scope. " http://docwiki.embarcadero.com/RADStudio/Seattle/en/Interface_References
    
    This is important where object destruction has side effects whose timing of execution is of significance. This documented Delphi feature pattern is used in Delphi code to cause actions to happen automatically at the end of a scope.
    
    With this patch Delphi mode compatibility turns this behaviour on, or it can be turned on via a compiler directive SCOPED_INTERFACE_DESTROY.

diff --git a/compiler/globals.pas b/compiler/globals.pas
index c5fef8b..47c7762 100644
--- a/compiler/globals.pas
+++ b/compiler/globals.pas
@@ -52,7 +52,7 @@ interface
          [m_delphi,m_all,m_class,m_objpas,m_result,m_string_pchar,
           m_pointer_2_procedure,m_autoderef,m_tp_procvar,m_initfinal,m_default_ansistring,
           m_out,m_default_para,m_duplicate_names,m_hintdirective,
-          m_property,m_default_inline,m_except,m_advanced_records];
+          m_property,m_default_inline,m_except,m_advanced_records,m_scoped_intf_destroy];
        fpcmodeswitches =
          [m_fpc,m_all,m_string_pchar,m_nested_comment,m_repeat_forward,
           m_cvar_support,m_initfinal,m_hintdirective,
diff --git a/compiler/globtype.pas b/compiler/globtype.pas
index 617a872..614c039 100644
--- a/compiler/globtype.pas
+++ b/compiler/globtype.pas
@@ -288,7 +288,10 @@ interface
          m_nested_procvars,     { support nested procedural variables }
          m_non_local_goto,      { support non local gotos (like iso pascal) }
          m_advanced_records,    { advanced record syntax with visibility sections, methods and properties }
-         m_isolike_unary_minus  { unary minus like in iso pascal: same precedence level as binary minus/plus }
+         m_isolike_unary_minus, { unary minus like in iso pascal: same precedence level as binary minus/plus }
+         m_scoped_intf_destroy  { destroy interface references when last reference
+                                  goes out of scope (Delphi emulation)
+                                  (versus freepascal behaviour that is immediately when its no longer referenced }
        );
        tmodeswitches = set of tmodeswitch;
 
@@ -391,7 +394,7 @@ interface
        pocall_default = pocall_stdcall;
 {$endif}
 
-       modeswitchstr : array[tmodeswitch] of string[18] = ('','',
+       modeswitchstr : array[tmodeswitch] of string[19] = ('','',
          '','','','','','',
          {$ifdef fpc_mode}'',{$endif}
          { more specific }
@@ -420,7 +423,8 @@ interface
          'NESTEDPROCVARS',
          'NONLOCALGOTO',
          'ADVANCEDRECORDS',
-         'ISOUNARYMINUS');
+         'ISOUNARYMINUS',
+         'SCOPED_INTERFACE_DESTROY');
 
 
      type
diff --git a/compiler/ncgcal.pas b/compiler/ncgcal.pas
index 5078438..f95ff13 100644
--- a/compiler/ncgcal.pas
+++ b/compiler/ncgcal.pas
@@ -432,9 +432,12 @@ implementation
             case location.loc of
               LOC_REFERENCE :
                 begin
-                  if is_managed_type(resultdef) then
-                     cg.g_finalize(current_asmdata.CurrAsmList,resultdef,location.reference);
-                   tg.ungetiftemp(current_asmdata.CurrAsmList,location.reference);
+                  if not ((m_scoped_intf_destroy in current_settings.modeswitches) and is_interface(resultdef)) then
+                  begin
+                    if is_managed_type(resultdef) then
+                       cg.g_finalize(current_asmdata.CurrAsmList,resultdef,location.reference);
+                     tg.ungetiftemp(current_asmdata.CurrAsmList,location.reference);
+                  end;
                 end;
 {$ifdef x86}
               LOC_FPUREGISTER :
intf.patch (4,217 bytes)   

Jared Davison

2016-07-25 07:45

reporter  

checkcompilerinterfacerefcountsemantics.pas (1,303 bytes)

Jared Davison

2016-07-25 07:47

reporter   ~0093826

Note that the patch is based on release_2_6_4

Michael Van Canneyt

2016-07-25 09:37

administrator   ~0093827

The problem is 'last reference goes out of scope' :

The behaviour of temp variables used to evaluate expressions is undefined and not documented. So by definition, you cannot say when the last reference goes out of scope, since the amount of references and their lifetime is compiler-implementation-determined.

Thaddy de Koning

2016-07-25 17:16

reporter   ~0093828

Last edited: 2016-07-25 17:18

View 2 revisions

 Note that this patch can cause more easily use-after-free to be caught at the wrong moment.
Scope is not a matter of opinion.
Reference equals zero: scope ends. is a valid paradigm. Hence that may be compiler specific, even between Delphi versions (since their back-end changed, did you check that?).
I believe the current FPC implementation is more logical than the Delphi one.

Jared Davison

2016-07-26 04:00

reporter   ~0093832

We agree this is an undocumented behaviour of Delphi.

We believe there are a lot of libraries that exploit this behaviour.

We have found this behaviour consistent across all Delphi version include 10.1 Berlin.

We believe it is important for cross platform library maintainability.

If it is undefined, doing it the same as Delphi when in Delphi compatibility mode makes it easier to move code to free pascal.

The language feature of calling a function without assigning it to a variable is a feature of Delphi as this is not allowed in standard pascal.

The patch allows this to be configured via pragma and compatibility mode so it would be reasonable as not to affect pure free pascal implementation.

Thaddy de Koning

2016-07-26 07:43

reporter   ~0093833

Last edited: 2016-07-26 07:51

View 4 revisions

 "The language feature of calling a function without assigning it to a variable is a feature of Delphi as this is not allowed in standard pascal."

It is also a feature of Freepascal and is the default in {$MODE DELPHI}.
In other modes it can be enabled by {$EXTENDEDSYNTAX ON}

Btw: you are using a rather old and unsupported compiler.
If compatibity with Delphi is important change it to 3.0.0. That is supported and the default until the maintenance release 3.0.2 (soon?)

This is much more compatible. If the behavior is still the same you might submit a patch against 3.0.1 (soon to be frozen) or better:trunk.

This internal refcount semantics issue has come up before, but I can't find the mantis entries.
It was closed with no change required.

Martok

2016-07-26 08:41

reporter   ~0093834

Re: refcounting internal variables: 25990 was my entry into that neverending story, you can find others from there.
You can summarize this as "in FPC mode, Reference equals zero: scope ends. Only you have no way of knowing/controlling when that is, so better expect scope".

Thaddy de Koning

2016-07-26 10:18

reporter   ~0093836

Last edited: 2016-07-26 10:23

View 2 revisions

 As Florian states in the related reports the Delphi implementation is more error-prone. I already referred to the use-after-free issue that can more easily be caused in the Delphi implementation, where it is actually hidden and shows up much later.

Is it a good idea to change a probably superior implementation into an inferior one?

In your case it is probably better to implement an observer pattern, where the observer triggers the required action when all references are released. That is both Delphi and Freepascal compatible.

syfre

2017-01-02 08:57

reporter   ~0097224

I understand the answer of Michael, and i could agree, but i think this break a lot of delphi code, this is a test case with a similar problem with the as operator :

procedure TTestInterfacedObjectPatterns.Test_ObjectByRefCount_as_Pattern;
var obj:TmyObjectByRefCount; intf:ImyInterface;
begin
  {$IFDEF FPC}
  // FPC: failed because ref counted temp variable
  Check(false,'FPC Will fail because of as operator');
  {$ENDIF}

  glbObjectCount:=0;

  obj := TmyObjectByRefCount.Create;
  Check(glbObjectCount=1,Format('ObjectCount %d, expected 1',[glbObjectCount]));
  Check(obj.RefCount=1,Format('obj.RefCount %d, expected 1',[obj.RefCount]));

  intf := obj as ImyInterface;
  // FPC: as operator assign a hidden temp variable, reference twice the object
  // FPC: failed because ref counted temp variable RefCount=3
  Check(obj.RefCount=2,Format('obj.RefCount %d, expected 2',[obj.RefCount]));

  obj._Release;
  Check(glbObjectCount=1,Format('ObjectCount %d, expected 1',[glbObjectCount]));
  Check(obj.RefCount=1,Format('obj.RefCount %d, expected 2',[obj.RefCount]));

  intf._Release;
  // FPC: failed because ref counted temp variable still reference the object
  Check(glbObjectCount=0,Format('ObjectCount %d, expected 0',[glbObjectCount]));
  Pointer(intf) := nil;
  // FPC: will desallocated the object in the end block
end;

In this test case, the as operator allocates a reference counted temp variable which retains the object until the end block.

Note that is specific to "as interface" operator, the same test using "supports()" don't break because "supports" doesn't allocated this hidden temp variable :

procedure TTestInterfacedObjectPatterns.Test_ObjectByRefCount_support_Pattern;
var obj:TmyObjectByRefCount; intf:ImyInterface;
begin
  glbObjectCount:=0;

  obj := TmyObjectByRefCount.Create;
  Check(glbObjectCount=1,Format('ObjectCount %d, expected 1',[glbObjectCount]));
  Check(obj.RefCount=1,Format('obj.RefCount %d, expected 1',[obj.RefCount]));

  Check(Supports(obj,ImyInterface,intf));
  Check(obj.RefCount=2,Format('obj.RefCount %d, expected 2',[obj.RefCount]));

  obj._Release;
  Check(glbObjectCount=1,Format('ObjectCount %d, expected 1',[glbObjectCount]));
  Check(obj.RefCount=1,Format('obj.RefCount %d, expected 2',[obj.RefCount]));

  intf._Release;
  Check(glbObjectCount=0,Format('ObjectCount %d, expected 0',[glbObjectCount]));
  Pointer(intf) := nil;
end;


In addition to breaking delphi code, "intf := obj as Ixxx" and "Supports(obj,Ixxx,intf)" result in different behaviors. There are also performance impacts because of the _AddRef _Release additional calls.

Marco van de Voort

2017-01-02 11:12

manager   ~0097227

It has come up a dozen times in as many years. It should not be exaggerated, also any Delphi faq warns against mixing objects and interfaces. So it is simply bad code.

From what I can remember from earlier discussions, in very large/complex procedures some Delphi versions might also not wait till the end of the procedure to release so also on Delphi the rule is not as universal as people think.

Thaddy de Koning

2017-01-02 15:39

reporter   ~0097233

Yes, but I would like to see if Jared has still an issue.
I don't believe there is one. I suggest to close.

Jared Davison

2018-10-04 09:43

reporter   ~0111232

What I do know is that it took many years to discover this issue after porting a program from Delphi to FreePascal. There are programmers who write code exploiting the Delphi behaviour whether that is correct or not it doesn't matter, as the code is buried in millions of lines of code, no one would think to look there.

Unless one has unit tests that uncover the exact issue then it is unlikely to be caught.

An example of typical use that I've seen is:

procedure DoSomethingRequringThreadSafety;
begin
  AutoLockResource; // acquires a critical section and returns a reference interfaced counted object in temporary variable
  MutateSharedResources;
  // object returned by AutoLockResource calls destroy here implicitly allowing programmer to avoid writing the release line.
end;

function AutoLockResource: IAutoLock;
begin
  Result := TAutoLock.Create; // TAutoLock.Create acquires a global critical section, and in destroy releases it
end;

At the least, it is probably worth documenting that the behaviour is unsupported. The patch that we provided was designed to be enabled through a compiler directive so wouldn't affect be on by default to me that seems reasonable.

The patch was offered so that it could assist others if they encountered the issue they could enable the directive.

Perhaps it could be clearly noted in documentation that code such as the above is unsafe.

For now I'm just maintaining this patch privately since it hasn't been merged.

Thaddy de Koning

2018-10-04 15:54

reporter   ~0111241

That example simply works when the temp variable is a reference instantiated as an interface.
This was also discussed: it goes wrong when mixing class and interface instantiation but that is bad (really bad!) programming. It won't go wrong if the programmer is consistent: either class instantiation OR interface instantiation, but do not mix both.

BTW I work and worked with huge codebases and this is easy to mitigate. You can even automate it.

Thaddy de Koning

2018-10-04 15:55

reporter   ~0111242

Last edited: 2018-10-04 15:56

View 2 revisions

 Note the "assigned to" should be changed for this entry..Or reverted to unassigned?

Michael Van Canneyt

2018-10-04 16:17

administrator   ~0111244

As shown with the example program in 0006035, any assumptions about lifetimes of interfaces are wrong even in Delphi.


The sample program there was made in 2005.
Even with Delphi 10.2 (Tokio) the example crashes in Delphi.
This shows that solutions such as SCOPED_INTERFACE_DESTROY are not sufficient or desirable:

Attempting to emulate implementation specific behaviour is a never-ending chase, which we will not undertake.

Issue History

Date Modified Username Field Change
2016-07-25 07:43 Jared Davison New Issue
2016-07-25 07:43 Jared Davison File Added: intf.patch
2016-07-25 07:45 Jared Davison File Added: checkcompilerinterfacerefcountsemantics.pas
2016-07-25 07:47 Jared Davison Note Added: 0093826
2016-07-25 09:37 Michael Van Canneyt Note Added: 0093827
2016-07-25 17:16 Thaddy de Koning Note Added: 0093828
2016-07-25 17:18 Thaddy de Koning Note Edited: 0093828 View Revisions
2016-07-26 04:00 Jared Davison Note Added: 0093832
2016-07-26 07:43 Thaddy de Koning Note Added: 0093833
2016-07-26 07:45 Thaddy de Koning Note Edited: 0093833 View Revisions
2016-07-26 07:47 Thaddy de Koning Note Edited: 0093833 View Revisions
2016-07-26 07:51 Thaddy de Koning Note Edited: 0093833 View Revisions
2016-07-26 08:41 Martok Note Added: 0093834
2016-07-26 10:18 Thaddy de Koning Note Added: 0093836
2016-07-26 10:23 Thaddy de Koning Note Edited: 0093836 View Revisions
2017-01-02 08:57 syfre Note Added: 0097224
2017-01-02 11:12 Marco van de Voort Note Added: 0097227
2017-01-02 15:39 Thaddy de Koning Note Added: 0097233
2018-01-30 00:40 Maciej Izak Assigned To => Maciej Izak
2018-01-30 00:40 Maciej Izak Status new => assigned
2018-10-04 09:43 Jared Davison Note Added: 0111232
2018-10-04 15:54 Thaddy de Koning Note Added: 0111241
2018-10-04 15:55 Thaddy de Koning Note Added: 0111242
2018-10-04 15:56 Thaddy de Koning Note Edited: 0111242 View Revisions
2018-10-04 16:10 Michael Van Canneyt Assigned To Maciej Izak => Michael Van Canneyt
2018-10-04 16:10 Michael Van Canneyt Relationship added related to 0026602
2018-10-04 16:10 Michael Van Canneyt Relationship added related to 0025990
2018-10-04 16:11 Michael Van Canneyt Relationship added related to 0009472
2018-10-04 16:11 Michael Van Canneyt Relationship added related to 0006035
2018-10-04 16:17 Michael Van Canneyt Note Added: 0111244
2018-10-04 16:17 Michael Van Canneyt Status assigned => resolved
2018-10-04 16:17 Michael Van Canneyt Resolution open => no change required