stdcall functions with values returned-by-pointer-arg have incorrectly decorated names

[wesleywiser]

Functions using the stdcall calling convention return values in one of three ways (wikipedia):

• 32 bits or smaller: in EAX register
• 33-64 bits: in EAX and EDX registers
• 65 bits or larger: by a pointer passed into the function from the calling code

In the third case, the pointer is not considered to be part of the argument list size for the purposes of the @size name mangling suffix. For example:

#include <stdint.h>

struct more_complex
{
    int64_t a;
    int64_t b;
    int64_t c;
};

extern "C" __declspec(dllexport) more_complex __stdcall return_more_complex()
{
    return { 123, 456, 789 };
}

D:\code\temp\link-rs>dumpbin /exports export.lib
Microsoft (R) COFF/PE Dumper Version 14.30.30528.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file export.lib

File Type: LIBRARY

     Exports

       ordinal    name

                  _return_more_complex@0

However, if we try to link to this function from Rust:

#[derive(PartialEq)]
#[repr(C)]
struct more_complex {
    a: i64,
    b: i64,
    c: i64,
}

#[link(name = "export")]
extern "system" {
    fn return_more_complex() -> more_complex;
}

fn main() {
    unsafe {
        assert_eq!(return_more_complex(), more_complex { a: 123, b: 456, c: 789 });
        println!("ok");
    }
}

Then we get a linker error because the argument list size is incorrectly calculated as 4 instead of 0:

D:\code\temp\link-rs>rustc import.rs --target i686-pc-windows-msvc
error: linking with `link.exe` failed: exit code: 1120
  |
  = note: import.import.25f85bb4-cgu.8.rcgu.o : error LNK2019: unresolved external symbol __imp__return_more_complex@4 referenced in function __ZN6import4main17h0ecc22ee2d1aa233E
          import.exe : fatal error LNK1120: 1 unresolved externals

Thanks to @kennykerr for the repro!

[workingjubilee]

Does this only affect extern "system" or does it also affect extern "C" on 32-bit Windows?
Probably not the place to ask, but: extern "C" and extern "system" on x86_64-pc-windows-msvc both follow the x64 default calling convention (which functionally extends the __fastcall convention), as it is the default calling convention for the platform, right?

[wesleywiser]

Does this only affect extern "system" or does it also affect extern "C" on 32-bit Windows?

I think extern "C" on 32-bit Windows would be the __cdecl calling convention which doesn't append the parameter size in bytes to the decorated name so there should be no issue there. I believe the affected calling conventions are __stdcall, __fastcall and __vectorcall.

Probably not the place to ask, but: extern "C" and extern "system" on x86_64-pc-windows-msvc both follow the x64 default calling convention (which functionally extends the __fastcall convention), as it is the default calling convention for the platform, right?

That should be the case, yes. AFAIK, the only supported calling conventions on x86_64-pc-windows-msvc are "the x64 calling convention" and __vectorcall.

[luqmana]

Probably need to update LLVM here:

https://github.com/llvm/llvm-project/blob/595c418ad6a1230103000dcebdb158129d033077/llvm/lib/IR/Mangler.cpp#L92-L112

I imagine something like this in the loop would work:

if (A.hasStructRetAttr() && A.getDereferenceableBytes() > 8)
  continue;

[wesleywiser]

@luqmana Based on the LLVM Lang Ref, it read to me like just the presence of the sret attribute would be sufficient to indicate the parameter size should not be included. Therefore my patch looks like this:

diff --git a/llvm/lib/IR/Mangler.cpp b/llvm/lib/IR/Mangler.cpp
index bbdde586e6e0..0a61174c638c 100644
--- a/llvm/lib/IR/Mangler.cpp
+++ b/llvm/lib/IR/Mangler.cpp
@@ -99,6 +99,11 @@ static void addByteCountSuffix(raw_ostream &OS, const Function *F,
   const unsigned PtrSize = DL.getPointerSize();

   for (const Argument &A : F->args()) {
+    // For the purposes of the byte count suffix, structs returned by pointer
+    // do not count as function arguments.
+    if (A.hasStructRetAttr())
+      continue;
+
     // 'Dereference' type in case of byval or inalloca parameter attribute.
     uint64_t AllocSize = A.hasPassPointeeByValueCopyAttr() ?
       A.getPassPointeeByValueCopySize(DL) :
diff --git a/llvm/test/CodeGen/X86/stdcall.ll b/llvm/test/CodeGen/X86/stdcall.ll
index 3cefe14fe0d5..25107c5d93f2 100644
--- a/llvm/test/CodeGen/X86/stdcall.ll
+++ b/llvm/test/CodeGen/X86/stdcall.ll
@@ -18,6 +18,21 @@ entry:
   ret i32 %a
 }

+%struct.large_type = type { i64, i64, i64 }
+
+define x86_stdcallcc void @ReturnLargeType(%struct.large_type* noalias nocapture sret(%struct.large_type) align 8 %agg.result) {
+; CHECK: ReturnLargeType@0:
+; CHECK: retl
+entry:
+  %a = getelementptr inbounds %struct.large_type, %struct.large_type* %agg.result, i32 0, i32 0
+  store i64 123, i64* %a, align 8
+  %b = getelementptr inbounds %struct.large_type, %struct.large_type* %agg.result, i32 0, i32 1
+  store i64 456, i64* %b, align 8
+  %c = getelementptr inbounds %struct.large_type, %struct.large_type* %agg.result, i32 0, i32 2
+  store i64 789, i64* %c, align 8
+  ret void
+}
+
 @B = global %0 { void (...)* bitcast (void ()* @MyFunc to void (...)*) }, align 4
 ; CHECK: _B:
 ; CHECK: .long _MyFunc@0
diff --git a/llvm/test/CodeGen/X86/vectorcall.ll b/llvm/test/CodeGen/X86/vectorcall.ll
index d3d44f6bdd7a..b8d53eaf0bba 100644
--- a/llvm/test/CodeGen/X86/vectorcall.ll
+++ b/llvm/test/CodeGen/X86/vectorcall.ll
@@ -172,8 +172,7 @@ declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture r
 declare void @llvm.memcpy.p0i8.p0i8.i32(i8* nocapture writeonly, i8* nocapture readonly, i32, i1)

 define x86_vectorcallcc void @test_mixed_7(%struct.HVA5* noalias sret(%struct.HVA5) %agg.result) {
-; X86-LABEL: test_mixed_7@@4
-; X64-LABEL: test_mixed_7@@8
+; CHECK-LABEL: test_mixed_7@@0
 ; X64:         mov{{[ql]}}     %rcx, %rax
 ; CHECK:       movaps  %xmm{{[0-9]}}, 64(%{{rcx|eax}})
 ; CHECK:       movaps  %xmm{{[0-9]}}, 48(%{{rcx|eax}})

Does that seem reasonable to you or do you think we should include the additional check for A.getDereferenceableBytes() > 8?

[luqmana]

Ah, you're right, just checking for sret should suffice.