16

I recently came across (again) the Delphi compiler code-gen bug when passing an interface as const leaks a reference.

This happens if your method is declared to pass an interface variable as const, e.g.:

procedure Frob(const Grob: IGrobber);

and the fix is to simply remove the const:

procedure Frob(Grob: IGrobber);

I understand that const (and var, and out) allow you to pass items by reference. In the case of a structure this saves an argument copy; letting you instead simply pass the pointer to the item.

In the case of an Object/Pointer/Interface there is no need to pass by reference, since it is a reference; it already will fit in a register.

In order to never have this problem again, i went on a crusade. I searched all my source trees for:

const [A-Za-z]+\: I[A-Z]

And i removed about 150 instances where i pass an interface as const.

But there are ones that i cannot change. The TWebBrowser callback events are declared as:

OnDocumentComplete(Sender: TObject; const pDisp: IDispatch; var URL: OleVariant);
                                    \___/
                                      |
                                      ?

Have i gone too far? Have i done a bad thing?

Edit: Or, to phrase it in a less "based on opinion" style question: are there any serious down-sides to not passing an interface as const?

Bonus: When Delphi does not (always) increment a interface reference count they are violating The Rules of COM:

Reference-Counting Rules

Rule 1: AddRef must be called for every new copy of an interface pointer, and Release called for every destruction of an interface pointer, except where subsequent rules explicitly permit otherwise.

Rule 2: Special knowledge on the part of a piece of code of the relationships of the beginnings and the endings of the lifetimes of two or more copies of an interface pointer can allow AddRef/Release pairs to be omitted.

So while it may be an optimization that the compiler can take advantage of, it has to do it correctly so as to not violate the rules.

Community
  • 1
  • 1
Ian Boyd
  • 246,734
  • 253
  • 869
  • 1,219
  • 4
    Embarcadero have known about this for years. Danny Thorpe and Barry Kelly both told me it was a defect. More recently Marco came out with some weasling on Google+ to explain why it wasn't trivial to change. I guess that they have an implementation choice in the compiler that makes this hard to fix without a significant redesign and that they don't really know how to do that redesign. – David Heffernan Jun 24 '15 at 15:01
  • 1
    I'm sure you know this, but the point of `const` with an Interface/string/dyn_array is not to avoid `pass_by_value`, but to avoid ref-counting and the hidden try-finally that goes with it. You dwell on the `pass_by_value` part in the question which muddles the issue. – Johan Jun 24 '15 at 16:22
  • 1
    @Johan That's an interesting point. Semantically `const` means that the parameter cannot be modified by the function's implementation. Reference counting etc. are optimisations that are made on the basis of knowing no modifications are made. If the compiler had more sense it would make the same optimisations even for value params simply by observing that no modifications were made. – David Heffernan Jun 24 '15 at 17:16
  • 2
    @IanBoyd: You don't need to mass-remove every `const` from your interface parameters. All you really need to do is search your code for any cases where you are `Create()`'ing an interfaced object inlined inside a function call, eg: `Func(TMyObject.Create)`. That is the only leak, not the `const` itself. – Remy Lebeau Jun 24 '15 at 18:01
  • 2
    @IanBoyd: Delphi is NOT violating COM rules. A function taking an interface pointer as input only IS NOT required to touch the interface's reference count. That would be wasted overhead if EVERY function call did that. Look at *any* COM example written in C/C++ (remember that COM is primarily designed for C/C++). `AddRef()` and `Release()` are only used if an interface parameter is being modified by the function, or the interface needs to persist beyond the function call. Otherwise, the interface parameter is treated as just a regular pointer. Delphi DOES NOT do that unless `const` is used. – Remy Lebeau Jun 24 '15 at 18:05
  • 1
    [Discussion on G+ about the issue in question](https://plus.google.com/+StefanGlienke/posts/SM1oVCNa98q) – Dalija Prasnikar Jun 24 '15 at 18:38
  • There could be downsides to not passing as const depending on how you implement the underlying objects. You might want to implement some additional logic in the _addref and _release. – Graymatter Jun 24 '15 at 18:46
  • 1
    We had a compiler bug in a situation like yours. https://stackoverflow.com/questions/30692000 – Gabriel Jun 28 '23 at 10:43

1 Answers1

20

This happens if your method is declared to pass an interface variable as const, e.g.:

procedure Frob(const Grob: IGrobber);

That's not quite right. In order for there to be a leak, you need for there to be nothing in the code that ever takes a reference to the newly created object. So if you write:

Frob(grob);

There there's no problem because the interface grob already has at least one reference.

The problem arises when you write:

Frob(TGrobberImplementer.Create);

In that scenario nothing takes a reference to the interface and so it is leaked. Well, it will be leaked so long as nothing in the implementation of Frob takes a reference to it.

Have I done a bad thing?

Well, that depends. I don't think anything particularly bad will come of what you have done. There is a downside in terms of performance because all of your functions that accept interface parameters will now have to add and release references, using implicit try/finally blocks. Only you can judge whether that matters.

The more significant issue relates to code that is outside of your control. You give

procedure OnDocumentComplete(Sender: TObject; const pDisp: IDispatch; var URL: OleVariant);

as an example. There's no issue there because you never call that method. It's an event handler that you implement. The framework calls it, and it passes an interface that is already referenced.

The real problem comes from methods declared in the RTL or any other third party code that you call. If you are calling the methods, and if they use const interface arguments then you can fall into the trap.

It's easy enough to work around, albeit tiresome.

grob := TGrobberImplementer.Create;
Frob(grob);

My reasoning to deal with the issue goes like this:

  1. There is a performance cost in passing interface parameters by value.
  2. I cannot ensure that every method that I call will accept interface parameters by value.
  3. Therefore, I embrace the fact that I need to deal with calling const interface parameters at least some of the time.
  4. Since I have to deal with it some of the time, and since I hate inconsistency, I choose to embrace dealing with it all of the time.
  5. Therefore, I choose to make all interface parameters in methods that I write be const.
  6. And consequently I make sure that I never pass an interface as an argument unless it is already referenced by a variable.
Community
  • 1
  • 1
David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
  • 5
    I wonder why dcc cannot make a hidden variable for interface expressions ( like `Frob(TGrobberImplementer.Create);` ) like it does for string expressions... – Arioch 'The Jun 24 '15 at 15:09
  • 1
    @Arioch'The Yes, it's rather lame isn't it. – David Heffernan Jun 24 '15 at 15:17
  • 1
    @Arioch'The: that would be an ideal solution, but they have already gone on record before saying that doing so is not easy for them to implement in the current DCC compiler architecture, which is why it has not been done yet. It is not something they can just slip in and be done with it, pieces of the compiler have to be redesigned to accommodate it, and they don't want to do that redesign yet. – Remy Lebeau Jun 24 '15 at 18:11
  • 7
    FWIW, instead of the two-liner and the extra variable at the end of your answer, you can do it in one line without the extra variable, but only with a cast: `Frob(TGrobberImplementer.Create as IGrobber);`. That will also increment the refcount to 1, like in your two-liner. – Rudy Velthuis Jun 24 '15 at 18:43
  • 2
    @RudyVelthuis I'd not thought of that, but it will happen because the compiler feels compelled to guard against the `as` failing and it still having to dispose of the original interface. And so it introduces an implicit local variable. I find it hard to believe that it is hard to change the compiler to fix this long standing bug. – David Heffernan Jun 24 '15 at 18:49
  • We just ran into this issue again. It would be great if there would be even a compiler hint/warning about this. – GolezTrol Sep 16 '19 at 09:40