JMP to expernal methods <> inlined call to external method, bug or correct design?

[Johan Bontes 11]

I have the following code (Delphi Berlin):
 

interface

type

  TSliver = record
  public
  private
    case integer of
      8: (Data8: int64);
  end;

  TSlice = record
  private
    FData: array [0..63] of byte;
  end;

  TSliverHelper = record helper for TSliver
    class function NS(const North, South: TSlice; out Changed: TSliverChanges): TSliver; static; inline;
  end;

  {$L 'C:\mydir\AVXGenerate.o'}  //imported assembly generated by lazarus.

  procedure AVXGENERATE_TSLIVERHELPER_NS; external name 'AVXGENERATE_$$_TSLIVERHELPER_NS'; //no parameters for simplicity.

implementation

class function TSliverHelper.NS(const North, South: TSlice; out Changed: TSliverChanges): TSliver; {inline;}
  //RCX =North: PSlice
  //RDX =South: PSlice
  //R8 = Changed: PSliverChanges ((scUnchanged=0, scChanged=1, scInvalid=3));
  //RAX = Result: TSliver (as Int64)
begin
  AVXGENERATE_TSLIVERHELPER_NS;
end;

//asm
//    jmp AVXGENERATE_TSLIVERHELPER_NS;
//end;

The asm method works correctly, the inlined method does not. If we look at the generated code it's easy to see why this happens.

 

    a:= TSliver.NSTest(N,S,statusA);
    //a = -1
    UnitTests.pas.877: b:= TSliver.NS(N,S, statusB);
    00000000009A77CE 488D8C2480000000 lea rcx,[rsp+$0080]
    00000000009A77D6 488D542440       lea rdx,[rsp+$40]
    00000000009A77DB 4C8D44242E       lea r8,[rsp+$2e]         
    00000000009A77E0 E82B5E0100       call TSliverHelper.NS
    00000000009A77E5 4889442430       mov [rsp+$30],rax      //b = 7 why?

    //Let's see what's happening in the inlined method.
    class function TSliverHelper.NS(const [ref] N,S: TBigRec; out status: TEnumRec): UInt64_Rec;
    Unit2.pas.4360: begin
    00000000009BD610 55               push rbp
    00000000009BD611 4883EC30         sub rsp,$30
    00000000009BD615 488BEC           mov rbp,rsp
    Unit2.pas.4361: AVXGENERATE_TSLIVERHELPER_NS;
    00000000009BD618 E833350000       call AVXGENERATE_TSLIVERHELPER_NS  //external asm procedure with no declared params.
    Unit2.pas.4362: end;              //At this point RAX = result = -1, all OK
    00000000009BD61D 488B4528         mov rax,[rbp+$28]   //Oops, RAX gets overwritten, why?  <<<<???????????????????????
    00000000009BD621 488D6530         lea rsp,[rbp+$30]
    00000000009BD625 5D               pop rbp
    00000000009BD626 C3               ret

Why does the compiler insert an assignment to RAX? I have not instructed it to do anything of the sort.

Compiling this code also does not issue a warning that Result might be unassigned.

 

Why does the compiler do this?

(I know I can fix this by declaring the external proc as a function, but that's beside the point).

 

EDIT

 

When we redefine the external proc as a function and change the inline method as shown below, correct code gets generated. It also gives some insight what is going on:

 

Unit2.pas.4360: begin
00000000009BD610 55               push rbp
00000000009BD611 4883EC30         sub rsp,$30
00000000009BD615 488BEC           mov rbp,rsp
Unit2.pas.4361: Result:= AVXGENERATE_TSLIVERHELPER_NS;
00000000009BD618 E833350000       call AVXGENERATE_TSLIVERHELPER_NS
00000000009BD61D 48894528         mov [rbp+$28],rax                  //pointless stack trashing
Unit2.pas.4362: end;
00000000009BD621 488B4528         mov rax,[rbp+$28]                  //the exit code expects Result to be in the stackframe.
00000000009BD625 488D6530         lea rsp,[rbp+$30]
00000000009BD629 5D               pop rbp
00000000009BD62A C3               ret

The compiler is just copy pasting its exit code, regardless of the actual code in the body.

Note that I've compiled the code with no stackframes

 

Edited November 16, 2018 by Johan Bontes

[Arnaud Bouchez 380]

I think the compiler is lost since you didn't put any parameter to your external function definition.

It is just the same for the result parameter in RAX as for regular parameters.

Try with the exact function definition including all parameters, and I hope (crossed fingers!) it will be fine.

 

The "stack frames" parameter in the Project Options is pointless on Win64 IIRC.

Only with an `asm .noframe` directive you may have no stack frame.

 

IMHO the redirection call won't be noticeable in performance, especially if you write:

asm
    .noframe
    jmp AVXGENERATE_TSLIVERHELPER_NS
end;

 

I wouldn't play with inline + assembly with Delphi, which has a long history of problems when inlining code...

Edited November 16, 2018 by Arnaud Bouchez

[David Heffernan 2291]

Bug in your code. The inliner has no reason to pass any parameters because you told it the external function has none.

 

If you want to use inline, and doing so removes the jmp leaving just a single call, then you need to declare the external function correctly. 

 

I think I already told you this on SO. 

[Kryvich 163]

Still, it is not clear why the compiler did not issue a warning about an unassigned Result. I minified the code to make it clearer.

program TestUnassignedResult;
{$APPTYPE CONSOLE}
type
  TRec = record
    Data: Byte;
    function NS: TRec;
  end;

function TRec.NS: TRec;
begin
  Writeln('I''m in NS');
  // W1035 Return value of function 'NS' might be undefined <-- not showed!!!
end;

begin
  //..
end.

 

[uligerhardt 18]

Does the compiler show any "undefined"warnings for records at all?

[Kryvich 163]

@uligerhardt It seems that it does not. Neither for records, nor for arrays.

function GetRec: TRec;
begin
  // Forgot to assign Result...
end;

function GetArr: TArray<Integer>;
begin
  // Forgot to assign Result...
end;

var
  rec: TRec;
  arr: TArray<Integer>;
begin
  rec.Data := 255;
  rec := GetRec;
  Writeln('rec.Data = ', rec.Data); // = 0

  SetLength(arr, 1000);
  arr := GetArr;
  Writeln('Length of arr = ', Length(arr)); // = 0
  Readln;
end.

 

[David Heffernan 2291]

Compiler warnings, rather missing the point 

[Hallvard Vassbotn 3]

That’s because functions with record (and other structured) return types are really implemented as hidden var-parameters. 

 

IMO, they should have been implemented as out-params, to get proper warnings. Alas, functions returning structured types preceded support for out-parameters (IIRC), and they never went back to fix this. 

[Stefan Glienke 1901]

6 hours ago, Hallvard Vassbotn said:

they should have been implemented as out-params

That would have caused a ton of unnecessary extra initializations because of out param.

If the compiler would actually be clever and detect them and omit when unnecessary I would agree.

Also if it would not be that terrible it could actually give the warning regardless the internal working of the parameter.

Edited December 9, 2018 by Stefan Glienke

[Hallvard Vassbotn 3]

I agree. Uninitialized Result in a function should give a warning. Period.