View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0009472 | FPC | Compiler | public | 2007-08-21 15:37 | 2018-10-04 16:11 |
| Reporter | Aleks Vtyurin | Assigned To | Yuriy Sydorov | ||
| Priority | normal | Severity | major | Reproducibility | always |
| Status | closed | Resolution | won't fix | ||
| Product Version | 2.2.0 | Product Build | |||
| Target Version | Fixed in Version | ||||
| Summary | 0009472: "as" increase .RefCount in INTERFACE | ||||
| Description | When use "as" for interface typecasts ".RefCount" increase. | ||||
| Additional Information | var obj: TInterfacedObject; iobj, iobj2: IUnknown; begin obj:= TInterfacedObject.Create; WriteLn('obj.RefCount=', obj.RefCount); // =0 iobj:= obj; WriteLn('obj.RefCount=', obj.RefCount); // =1 iobj2:= iobj as IUnknown; //Error!!! WriteLn('obj.RefCount=', obj.RefCount); // need 2, get 3 iobj2:= nil; WriteLn('obj.RefCount=', obj.RefCount); // need 1, get 2 iobj:= nil; WriteLn('obj.RefCount=', obj.RefCount); // need 0, get 1 end. | ||||
| Tags | No tags attached. | ||||
| Fixed in Revision | |||||
| FPCOldBugId | |||||
| FPCTarget | |||||
| Attached Files |
| ||||
| related to | 0008214 | closed | Florian | Returning interface from function is inefficient |
| has duplicate | 0011503 | closed | Jonas Maebe | Interfaces returned from functions are not properly released. |
| has duplicate | 0021460 | resolved | Jonas Maebe | As operator on an interface increment the refcount by two |
| has duplicate | 0025990 | resolved | Michael Van Canneyt | "as"-operator on interfaces references temporary variable |
| related to | 0006035 | closed | Jonas Maebe | AV by interface access |
| related to | 0016578 | closed | Jonas Maebe | Implicit create interface references |
| related to | 0030409 | resolved | Michael Van Canneyt | FPC delphi mode behaviour does not honour Delphi documented object lifetimes |
|
2007-08-21 15:37
|
project1.lpr (637 bytes) |
|
|
There is no leak however. The "as" is implemented as a function, the result of this function is returned in a temp, and this temp is obviously refcounted. The temp is freed at the end of the main program (or function, if you do this inside a function). I don't know when or how it really matters that the refcount temporarily increases because of this. The same happens if you call a function which returns the interfaced object. |
|
|
Jonas Maebe>The same happens if you call a function which returns the interfaced object. Yes. Error happens not only with "as", but with any "returns the interfaced object" Jonas Maebe>The temp is freed at the end of the main program Maybe need "temp:= nil" after returning? Example: ----------------------------- var obj: TInterfacedObject; iobj, iobj2: IUnknown; function RunAs: IUnknown; begin Result:= iobj as IUnknown; WriteLn('obj.RefCount=', obj.RefCount); // need 2, get 3 end; begin obj:= TInterfacedObject.Create; WriteLn('obj.RefCount=', obj.RefCount); // =0 iobj:= obj; WriteLn('obj.RefCount=', obj.RefCount); // =1 iobj2:= RunAs; // Error!!! WriteLn('obj.RefCount=', obj.RefCount); // need 2, get 3 iobj2:= nil; WriteLn('obj.RefCount=', obj.RefCount); // need 1, get 2 iobj:= nil; WriteLn('obj.RefCount=', obj.RefCount); // need 0, get 1 |
|
|
I still don't understand why it is an error, since no leaking occurs. I know it's different from Delphi, but that in itself does not make it wrong. Adding all those "temp:=nil" statements implicitly will mainly cause code bloat. |
|
|
1.By logic of "interface" using .RefCount must contain object link count. At example object link count only 2. If .RefCount reach 3 - it`s an error. 2.By logic of "interface" object must destroy when object link count equal 0. At example object link count equil 0, but object does not destroy - it`s an error. 3.A compatibility with Delphi and other languages. |
|
|
1. No, the refcount is 3 because of the temps. 2. No, the refcount is 1 because of the temps 3. Does this actually cause errors in real world programs, or does it just look strange because you expected something different? |
|
|
Without the temp := nil; you cannot force the destruction of the object by setting all your references it to nil, because the temp keeps the object referenced. So, in the current FPC implementation, there is no way to limit the lifetime of an object to only part of a function, procedure or method. |
|
|
My question still stands: is this actually a problem in real world apps? FWIW, you get exactly the same behaviour with ansistrings and any other refcounted type. |
|
|
In my application I manualy control removing object from full object list and destroing by .refcounter. I was compeled to replace "as" with "QueryInterface" call. I don`t sure of correct application work now. |
|
|
I personally think you are using a hack which only works by accident in Delphi, because the entire point of refcounting is that you leave everything to the underlying run time system and do not manually interfere. Basing your code on assumptions on how the refcouting actually is implemented is asking for trouble and inherently unsupported. But thanks for giving a real world example, although I don't agree it is a good reason to change our current implementation and therefore I do not intend to modify the current behaviour. |
|
|
For process .RefCount modification hacking is not necessary :) Necessary realize own IUnknown. Ex.: TMyInterfacedObject = class(TObject,IUnknown) protected frefcount : longint; function QueryInterface(const iid : tguid;out obj) : longint;stdcall; function _AddRef : longint;stdcall; virtual; function _Release : longint;stdcall; virtual; public property RefCount : longint read frefcount; end; |
|
|
This is memory leak example: procedure Step(aCounter: integer); var iobj, iobj2: IUnknown; begin if aCounter<1 then Exit; iobj:= TInterfacedObject.Create; iobj2:= iobj as IUnknown; // ... some code iobj:= nil; iobj2:= nil; //Error!! Object did not destroyed // ... some code Step(aCounter-1); end; begin Step(1000); end. |
|
|
This is lock error example: var iwr: IMyWriter; iswr: IMySuperWriter; ird: IMyReader; begin iwr:= TMyWriter.Create('C:\some_file.dat'); //Open file and locking iswr:= iwr as IMySuperWriter; iswr.Write(AnyData); iswr:= nil; //some code iwr:= nil; //I want to unlock file and close ird:= TMyReader.Create('C:\some_file.dat'); //Open file //Error! Because file is locking //... |
|
|
I think that the problem in all of your examples is that you are actually using manual reference counting (you could simply replace the nil assignments with calls to a method which decreases the reference count and then you'd have manual reference counting) on top of automatic reference counting, and are depending on the fact that the automatic reference counting follows you manual reference counting scheme. Either you use automatic reference counting and make no assumptions about how it works under hood (because that implementation may even change over time, or depending on the underlying run time environment), or you use manual reference counting and you are in full control of everything. |
|
|
The current behaviour FPC bring to error. I showed of several examples. My programming methods are not relation to it! |
|
|
Ref. counted types require temps which must be cleaning in implicit finalize statements and we consider the placing of these implicit finalize statements as up to the compiler as long as there is no leak and there is no leak in FPC. You may not make any assumptions about the current value of the ref. counter. The proper way of coding it would be some close method and leave the cleaning up of the variables to the compiler. |
|
|
I added a relation with some technical background why we use the temp. currently. |
|
|
Is it possible to release temps of refcounted types when the temps are no longer needed? It will solve this problem and make the code more resource efficient. For example, currently some huge string data can be left in temp after some string operations even if all real string variables are released already... |
|
|
It is possible, but this would cause code bloat and slow everything down (because of all the extra inserted finalization calls for temps). It's also not easy, because it is pretty difficult (if possible at all) to distinguish between a temp and a local variable or parameter inside an inlined function/procedure. |
|
|
Yes, this will add some extra code for complex procedures where temps are actively reused. But if temps are not reused in a procedure the code size will not grow because temp will be released when no longer needed, instead of end of the procedure. Ref decrement will be used, not Finalize, and it will not slow down execution. |
|
|
Either you always do it, or never, because we currently never know in advance whether a temp will be reused or not (temps are allocated ad hoc, not in a separate pass after code generation). Also, it would make the behaviour even less predictable (although, as explained before, I don't think anyone should make any assumptions whatsoever about this behaviour). The reason it slows down in the general situation (where temps are reused) is because 1) more code -> worse instruction cache behaviour 2) all ref counting assignment operations call the routine to decrease the reference count of the target before actually assigning (and since we cannot distinguish between variables and temps in all situations, you cannot skip this for temps where that would not be necessary in the alternative scenario -- and while that could also be changed, overall we have been moving towards unifying temps and variables more and more over the year rather than making them more different). This means you get two decref counts instead of one (although the second one will see the target is nil and not do anything further). |
|
|
Agree. It was not very good idea :) Just found that Delphi do temps finalization at procedure end as well. Therefore it is not real issue. This bug with "AS" can be easily fixed by changing fpc_intf_as function to procedure with out parameter (Delphi do it such way). No temps will be need and less code will be genearated. |
|
|
Aleks' latter examples show no sign of reference counting by hand. I guess the problem may be that the function has a pascal calling convention, hence the callee can't clean up the temp. If it were using cdecl the caller, rather than the callee could clean up, i.e. refdec the temp after the actual assignment and everything should work as expected. So I am inclined to agree with Aleks there's an issue here. |
|
|
I consider "assigning nil to all user-assigned references and then expecting that no more references exist" to be manual reference counting (as you are manually counting how many references are left and expect some action to be taken when you think that no references are left anymore). Automatic reference counting means that you put whatever action needs to be taken at destruction time into the destructor and for the rest fire and forget. You do not make any assumptions about when this destructor will be called, only that when it is called no references exist anymore to the reference counted object (and the run time system should take care of that for you, which it does). This has nothing to do with C vs Pascal. What you propose about decreasing the reference after the assignment is what Yury proposed initially. |
|
|
No, I think it is very much different. In this case the calling convention matters for the compiler, implementation-wise, because this call is/should be hidden from the user. The problem you have with changing the implementation seems to be the indetermined state of the temp. One way to fix this is having the cdecl calling convention and let the caller part of the code decide if it is ment to be "temp". you can't write the compiler in such a way using a calling convention that behaves inhibited, i.e. pascal or stdcall. That's the whole idea. the "as" operator should refdec the temp instance for interfaces. All others shouldn't. This cannot be achieved with "callee" type conventions as opposed to "caller" type conventions. |
|
|
> No, I think it is very much different. In this case the calling convention matters for the compiler, > implementation-wise, because this call is/should be hidden from the user. I disagree it has to be hidden any more than it is currently. As explained, the user should not rely on implementation behaviour when working with features such as reference counting. > The problem you have with changing the implementation seems to be the indetermined state of the > temp. One way to fix this is having the cdecl calling convention and let the caller part of the code > decide if it is ment to be "temp". No, that is not the problem. It is now already the caller which allocates the temp returned by "as“ (and by any other function). > you can't write the compiler in such a way using a calling convention that behaves inhibited, i.e. > pascal or stdcall. That's the whole idea. the "as" operator should refdec the temp instance for > interfaces. All others shouldn't. They actually should, according to Aleks. And I agree that would be consistent: if you patch the as-behaviour, there's no reason not to patch the function behaviour, since both lead to problems if you rely on no hidden references existing over the course of multiple statements. It's just that I disagree with the fact that such hidden references lingering around for a while is a bug. |
|
|
This is example of unforeseeable program behaviour: var iwr: IMyWriter; iswr: IMySuperWriter; begin iwr:= TMyWriter.Create('C:\some_file.dat'); //Open file and locking if ConditionIsTrue the begin iswr:= iwr as IMySuperWriter; iswr.Write(AnyData); iswr:= nil; end; iwr:= nil; //I want to unlock file and close //Is file locked? //Maybe locked, maybe unlocked :) |
|
|
As Florian mentioned earlier: if you want to unlock a file and close it, you should add a close method and call that. Automatic reference counting is simply not the right tool for that kind of job, because you have no control over how it works and when it does what. |
|
|
"iobj:= iobj as IUnknown;" where "iobj=nil" is temporary problem solution. Ex.: var obj: TInterfacedObject; iobj, iobj2: IUnknown; begin obj:= TInterfacedObject.Create; WriteLn('obj.RefCount=', obj.RefCount); // =0 iobj:= obj; WriteLn('obj.RefCount=', obj.RefCount); // =1 iobj2:= iobj as IUnknown; WriteLn('obj.RefCount=', obj.RefCount); // need 2, get 3 iobj2:= nil; WriteLn('obj.RefCount=', obj.RefCount); // need 1, get 2 iobj:= nil; WriteLn('obj.RefCount=', obj.RefCount); // need 0, get 1 {$IfDef FPC} iobj:= iobj as IUnknown; //iTemp:= nil; //Object destroyed! {$EndIf} |
|
|
I would strongly recommend against relying on this behaviour, because that's (again) just an implementation detail. While we will not change that behaviour just because we can (nor just to annoy you ;), it is not guaranteed to stay like this forever either. |
|
|
Ok. I assigned this bug to myself :) I'll change fpc_intf_as function to procedure with out parameter (Delphi do it such way). Beside fixing this bug there will be nice "side effects": No temps will be needed and less code will be genearated for "AS". :) |
|
|
Whatever you change, please make sure that tests/test/trangeob.pp still works afterwards (don't forget to specify the -CR command line option). |
|
|
> Whatever you change, please make sure that tests/test/trangeob.pp still works afterwards (don't forget to specify the -CR command line option). If -CR is critical f0or trangeob, we should duplicate the test using some include. @Yury: Please see the discussion regarding the related bug. The point is that delphi handles interface function results differently. |
|
|
Delphi has simple rule: if function result of interface type is assigned to local variable, temp is not created. Otherwise the temp is created like in FPC currently. We can implement the same behavior in FPC. Also do not create temps for compilerprocs result (compilerprocs does not modify global variables anyway). fpc_intf_as can be stayed as function in this case. |
|
|
IIRC the rule is not that simple. If a function has nested functions it uses a temp or something like this ... |
|
|
Delphi does not create temps for nested function results. It check only if destination variable is local and does not create temp if it is true. P.S. The same rule is used for results of variant type and also can be impemented in FPC to optimize variants. |
|
|
Also, if Delphi sometimes creates a temp and sometimes not, Aleks' problem (the one with regular function results being kept in temps) actually also exists in Delphi, but it's just more rarely triggered there. So I rest my case: it is unsafe to depend on no hidden temps existing anymore after you have manually cleared all your own references, regardless of the implementation. |
|
|
Oh, actually Delphi checks if _any_ local variables of interface type of parent function is accessed by nested function. And create temp in that case. It is not hard to do the same... |
|
|
Also: please create a test program with as many "special cases" (nested functions throwing exceptions etc) you can think of, with { %opt=-gh } at the top and "HaltOnNotReleased := true;" at the start, so that the chance of introducing memory leaks is reduced as much as possible. |
|
|
Jonas, my tests with Delphi shows that it is generally safe to rely that no hidden temps are created for local variables of interface type. But for global variables it is unsafe. |
|
|
> Also, if Delphi sometimes creates a temp and sometimes not, Aleks' problem (the one with regular function results being kept in tempts) actually also exists in Delphi, but it's just more rarely triggered there. Yes, that's why I didn't fix it so far. Delphi's behaviour is rather error prone imo and can be even considered buggy. |
|
|
Yury: something like that cannot be "generally safe". Either it is supported and documented to behave in a particular way, or it is not. Tests do not prove anything, at best they just demonstrate the currently implemented behaviour for the particular cases you tested. It is just plain wrong to depend on such implementation details of high level concepts. It's the same as depending on the header layout of allocated memory blocks, or even on the code generated for e.g. an inc-statement. We build a compiler which is compatible to a large extent with the Object Pascal dialect supported by Delphi. We do not build a Delphi code generator emulator (that would be just too limiting for us, not to mention that it would cost a lot of time -- just look at all the energy spent on this bug report). |
|
|
Yes. That's why I said "generally safe" instead of "safe" :) You should agree it will be more efficient to implement Delphi behavior for interfaces and variants - faster code in most cases. |
|
|
Sure, I'm not opposed to optimizations (at least if they don't introduce memory leaks in certain corner cases). But I just want to make clear that the refcounting behaviour that goes with it is a side effect, and not something which is a purpose on its own (nor guaranteed to remain that way, or even be the same on all platforms -- e.g. with an llvm backend, the reference tracking could in theory be done using its garbage collection intrinsics). |
|
|
Sure. It is dangerous to rely on such internal stuff. And I agree with Florian that if you want to make sure that interface var is cleaned on specific place of code you need to call some its method to do cleanup (close files, free memory, etc). By the way we need to document temps allocation for interface/variant/string results to treat "bugs" like this as documented "feature" :) |
|
|
The compiler optimizes now the assignments of interfaces to local variables in procedures. This is the best the compiler can safely do to reduce the refcount increases. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2007-08-21 15:37 | Aleks Vtyurin | New Issue | |
| 2007-08-21 15:37 | Aleks Vtyurin | File Added: project1.lpr | |
| 2007-08-30 19:50 | Jonas Maebe | Note Added: 0014346 | |
| 2007-08-31 18:56 | Aleks Vtyurin | Note Added: 0014364 | |
| 2007-08-31 19:11 | Jonas Maebe | Note Added: 0014365 | |
| 2007-09-01 22:56 | Aleks Vtyurin | Note Added: 0014393 | |
| 2007-09-01 23:09 | Jonas Maebe | Note Added: 0014395 | |
| 2007-09-01 23:13 | Vincent Snijders | Note Added: 0014396 | |
| 2007-09-02 09:47 | Jonas Maebe | Note Added: 0014401 | |
| 2007-09-02 10:18 | Aleks Vtyurin | Note Added: 0014402 | |
| 2007-09-02 10:34 | Jonas Maebe | Note Added: 0014403 | |
| 2007-09-02 18:59 | Aleks Vtyurin | Note Added: 0014412 | |
| 2007-09-03 07:35 | Aleks Vtyurin | Note Added: 0014424 | |
| 2007-09-03 07:50 | Aleks Vtyurin | Note Added: 0014425 | |
| 2007-09-03 12:49 | Aleks Vtyurin | Note Edited: 0014424 | |
| 2007-09-03 13:04 | Jonas Maebe | Note Added: 0014431 | |
| 2007-09-03 13:44 | Aleks Vtyurin | Note Added: 0014434 | |
| 2007-09-03 14:04 | Florian | Note Added: 0014436 | |
| 2007-09-03 14:04 | Florian | Relationship added | related to 0008214 |
| 2007-09-03 14:05 | Florian | Note Added: 0014437 | |
| 2007-09-03 14:28 | Yuriy Sydorov | Note Added: 0014439 | |
| 2007-09-03 14:57 | Jonas Maebe | Note Added: 0014442 | |
| 2007-09-03 15:21 | Yuriy Sydorov | Note Added: 0014443 | |
| 2007-09-03 15:34 | Jonas Maebe | Note Added: 0014447 | |
| 2007-09-03 15:35 | Jonas Maebe | Note Edited: 0014447 | |
| 2007-09-03 16:02 | Yuriy Sydorov | Note Added: 0014455 | |
| 2007-09-03 18:15 | Thaddy de Koning | Note Added: 0014458 | |
| 2007-09-03 18:34 | Jonas Maebe | Note Added: 0014461 | |
| 2007-09-03 18:46 | Thaddy de Koning | Note Added: 0014462 | |
| 2007-09-03 19:26 | Jonas Maebe | Note Added: 0014463 | |
| 2007-09-03 19:31 | Aleks Vtyurin | Note Added: 0014464 | |
| 2007-09-03 19:49 | Jonas Maebe | Note Added: 0014465 | |
| 2007-09-03 19:53 | Aleks Vtyurin | Note Added: 0014466 | |
| 2007-09-03 19:57 | Aleks Vtyurin | Note Edited: 0014466 | |
| 2007-09-03 20:11 | Jonas Maebe | Note Added: 0014467 | |
| 2007-09-04 08:02 | Aleks Vtyurin | Note Edited: 0014466 | |
| 2007-09-04 10:18 | Yuriy Sydorov | Status | new => assigned |
| 2007-09-04 10:18 | Yuriy Sydorov | Assigned To | => Yuriy Sydorov |
| 2007-09-04 10:20 | Yuriy Sydorov | Note Added: 0014493 | |
| 2007-09-04 10:34 | Jonas Maebe | Note Added: 0014494 | |
| 2007-09-04 10:38 | Florian | Note Added: 0014495 | |
| 2007-09-04 11:13 | Yuriy Sydorov | Note Added: 0014497 | |
| 2007-09-04 11:19 | Florian | Note Added: 0014499 | |
| 2007-09-04 11:24 | Yuriy Sydorov | Note Added: 0014500 | |
| 2007-09-04 11:27 | Jonas Maebe | Note Added: 0014501 | |
| 2007-09-04 11:28 | Yuriy Sydorov | Note Added: 0014502 | |
| 2007-09-04 11:29 | Yuriy Sydorov | Note Edited: 0014502 | |
| 2007-09-04 11:31 | Jonas Maebe | Note Added: 0014503 | |
| 2007-09-04 11:32 | Yuriy Sydorov | Note Added: 0014504 | |
| 2007-09-04 11:33 | Yuriy Sydorov | Note Edited: 0014504 | |
| 2007-09-04 11:34 | Yuriy Sydorov | Note Edited: 0014502 | |
| 2007-09-04 11:36 | Florian | Note Added: 0014505 | |
| 2007-09-04 11:55 | Jonas Maebe | Note Added: 0014507 | |
| 2007-09-04 12:27 | Yuriy Sydorov | Note Added: 0014508 | |
| 2007-09-04 12:41 | Jonas Maebe | Note Added: 0014510 | |
| 2007-09-04 13:18 | Yuriy Sydorov | Note Added: 0014512 | |
| 2007-09-04 13:19 | Yuriy Sydorov | Note Edited: 0014512 | |
| 2007-09-07 12:45 | Jonas Maebe | Note Edited: 0014501 | |
| 2007-10-16 23:22 | Peter Vreman | Status | assigned => closed |
| 2007-10-16 23:22 | Peter Vreman | Note Added: 0015548 | |
| 2007-10-16 23:22 | Peter Vreman | Resolution | open => won't fix |
| 2008-06-19 09:39 | Jonas Maebe | Relationship added | has duplicate 0011503 |
| 2009-06-21 11:05 | Jonas Maebe | Relationship added | related to 0006035 |
| 2010-05-27 11:49 | Jonas Maebe | Relationship added | related to 0016578 |
| 2012-03-13 14:33 | Jonas Maebe | Relationship added | has duplicate 0021460 |
| 2014-04-08 17:27 | Jonas Maebe | Relationship added | has duplicate 0025990 |
| 2018-10-04 16:11 | Michael Van Canneyt | Relationship added | related to 0030409 |
