2

I present you a bug in the Delphi 5 compiler. I know there's not going to be any fix for it; but a workaround would be super

program Project1;

uses
  Dialogs, SysUtils;

{$R *.RES}

type
    IFoo = interface
        ['{D68DA49A-F870-433D-9343-4964BFECFF27}']
        procedure Grob(a: Integer; b: Integer);
    end;

    TFoo = class(TInterfacedObject, IFoo)
    public
        procedure Grob(a: Integer; b: Integer); virtual;
    end;

procedure TFoo.Grob(a: Integer; b: Integer);
begin

end;

function DoStuff(): Integer;
var
    foo: IFoo;
begin
    foo := TFoo.Create;
    try
        Result := 1;
        Exit;
    finally
        foo.Grob(0, 0);
    end;

    Result := 2;
end;

var
    n: Integer;
begin
    n := DoStuff;
    if n <> 0 then
        ShowMessage('Failed: '+IntToStr(n))
    else
        ShowMessage('Passed: '+IntToStr(n));

end.

The real guts is the function DoStuff which should return one:

function DoStuff(): Integer;
var
    foo: IFoo;
begin
    foo := TFoo.Create;
    try
        Result := 1;
        Exit;
    finally
        foo.Grob(0, 0);
    end;

    Result := 2;
end;

The function should return one. Instead it returns the address of the interfaced object:

enter image description here

The assembly

The code actually does start to set the result to one:

Project1.dpr.30: Result := 1;
    mov ebx,$00000001     ; place return value 1 in EBX
Project1.dpr.31: Exit;
    call @TryFinallyExit  ; call the finally block
    jmp DoStuff + $6E

and as the function is about to return, it does copy EBX into EAX in order to return it:

    mov eax,ebx           ;EBX into EAX for return

But finally block (calling the interfaced method) is the problem. It blows away the return value stored in EBX:

We arrive here from the call @TryFinallyExit
Project1.dpr.33: foo.Grob(0, 0);
    xor ecx,ecx
    xor edx,edx
    mov eax,[ebp-$04]
    mov ebx,[eax]  <----- overwriting ebx with interface address
    call dword ptr [ebx+$0c]
    ret

After the "call" to the finally block, it returns to a jump, which sends it to:

Project1.dpr.36: Result := 2;
...
    xor eax,eax
    pop edx
    pop ecx
    pop ecx
    mov fs:[eax],edx
    push $00442e1f
    lea eax,[ebp-$04]
    call @IntfClear
    ret
...
    mov eax,ebx  <----- places overwritten EBX into EAX for return
Project1.dpr.37: end;
    pop ebx
    pop ecx
    pop ebp
    ret

The return value rather than being one, or two, is the address of the interface pointer.

I know none of you have Delphi 5. And even if you did,

"What would you like me to say?"

I know the difficulty. What i actually need is some sort of workaround.

Ian Boyd
  • 246,734
  • 253
  • 869
  • 1,219
  • 1
    The problem I see here, is that we can produce a workaround to this code, but then it likely won't help with the real code ........ – David Heffernan Sep 11 '15 at 18:36
  • I would try making "grob" not virtual and see if it fixes the problem. If it does, you can always make it call a virtual "DoGrod" or something similar. – Ken Bourassa Sep 11 '15 at 20:11

1 Answers1

3

As you observed, the compiler is storing the result into EBX, but then overwriting it before it subsequently copies EBX into EAX to return the result to the caller.

The compiler should be doing one of the following:

  1. Using a different register to store the result value temporarily, so that its use of EBX does not destroy the result value, or
  2. Not using EBX in the call to Grob, or
  3. Storing the result value in something more persistent than a register, like on the stack.

Obviously options 1 and 2 are not readily available to you, but the latter is the workaround that you need to implement in this example – use a local variable to hold your intended Result value until you are ready to return it:

function DoStuff(): Integer;
var
  foo: IFoo;
  MyResult: Integer;
begin
  foo := TFoo.Create;
  try
    try
      MyResult := 1;
      Exit;
    finally
      foo.Grob(0, 0);
    end;

    MyResult := 2;
  finally
    Result := MyResult;
  end;
end;
David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • That's pretty elegant. It looks like the compiler doesn't "see" what the function `@TryFinallyExit` will do. As a result it doesn't realize it needs to protect `EBX`. – Ian Boyd Sep 12 '15 at 01:38
  • `TryFinallyExit()` is not the one modifying EBX, though. `Grob()` is. Nothing the compiler can do about that. It cannot "look into" `Grob()`'s implementation when deciding where to store the `Result` before the `try` block exits. Better would be for the compiler to know that it stored the `Result` in `EBX`, so it needs to preserve `EBX` during any `CALL` statements it makes before the final `RET` is reached at the end of `DoSStuff()`. – Remy Lebeau Sep 12 '15 at 03:00
  • That's not true. EBX must be preserved. The volatile registers are EAX, ECX and EDX. – David Heffernan Sep 12 '15 at 06:22
  • 1
    @DavidHeffernan: "EBX must be preserved" - Not according to Microsoft: [Using and Preserving Registers in Inline Assembly](https://msdn.microsoft.com/en-us/library/k1a8ss06.aspx): "*When using __asm to write assembly language in C/C++ functions, you don't need to preserve the EAX, **EBX**, ECX, EDX, ESI, or EDI registers*" – Remy Lebeau Sep 12 '15 at 06:41
  • @Remy You are reading the wrong docs. This is not MS inline asm, so the doc you quote is not relevant. We are talking about a function call. See https://msdn.microsoft.com/en-us/library/windows/hardware/ff561502(v=vs.85).asp or indeed http://docwiki.embarcadero.com/RADStudio/en/Program_Control#Register_saving_conventions – David Heffernan Sep 12 '15 at 06:45
  • The workaround works right up until the compiler decides to put MyResult in a register and thinks that EBX is a safe choice. Any change to the code could lead to the bug re-surfacing. A general, works everywhere workaround is probably not feasible. – David Heffernan Sep 12 '15 at 06:47
  • @DavidHeffernan: your MSDN link is broken. But I see this on the Embarcadero link: "*Procedures and functions must preserve the **EBX**, ESI, EDI, and EBP registers, but can modify the EAX, EDX, and ECX registers.*". In that regard, the `TryFinallyExit()` implementation is broken. – Remy Lebeau Sep 12 '15 at 06:49
  • This one works https://msdn.microsoft.com/en-us/library/windows/hardware/ff561502(v=vs.85).aspx The point is of course that the function call ABI is defined by the platform rather than the compiler vendor. Otherwise we'd never be able to call functions from other languages. Intel defined the abi a looong time ago. An edit is really needed to the answer. – David Heffernan Sep 12 '15 at 06:53
  • Expanding on the discussion of MS inline asm, all that doc is saying is that you can trash the registers if you want and the compiler will work out which ones you trashed and write in prolog/epilog to save/restore any that need to be preserved. If you don't trash a register that's not used elsewhere, the compiler won't write prolog/epilog. So the MS compiler can analyse the inline asm's register use. – David Heffernan Sep 12 '15 at 07:01
  • @RemyLebeau What i meant is that during the *"black box call to @TryFinallyExit"*, the value in EBX is being modified. It seems like the Delphi 5 compiler was unable to realize that it could peer into that *"black box"*, realize that `Grob` was going to be called, recognize that `Grob` would modify `EBX`, and save it. It seems like the compiler was stymied by the `call @TryFinallyExit`. – Ian Boyd Sep 12 '15 at 18:23
  • @IanBoyd: No, it could not "peer" into code like that. It was simply not following the documented rules that David pointed out. `Grob()` is modifying `EBX`, so it is responsible for preserving EBX before modifying it and then restoring it before exiting. It is not the responsibility of `TryFinallyExit()` to preserve EBX when calling `Grob()`, or the responsibility of `DoStuff()` to preserve EBX when calling `TryFinallyExit()` (though `DoStuff()` should be preserving EBX before using it for the `Result`). – Remy Lebeau Sep 12 '15 at 19:05
  • @RemyLebeau In reality, since it is all the one compiler, it should be able to notice what it is doing after it calls this function that it call, since it's the one that generated the code that it called after it calls what it calls :P – Ian Boyd Sep 13 '15 at 03:23
  • @Ian It's a clear compiler bug. Obviously it shouldn't stomp on its registers. *"What would you like us to say?"* – David Heffernan Sep 13 '15 at 07:33