[clang][#52782] Bail on incomplete parameter type in stdcall name mangling

[zero9178]

Summary

stdcall name mangling requires a suffix with the number equal to the sum of the byte count of all parameter types. In the case of a function prototype that has a parameter type of an incomplete type it is impossible to get the size of the type. While such a function is not callable or able to be defined in the TU, it may still be mangled when generating debug info, which would previously lead to a crash.
This patch fixes that by simply bailing out of the loop and using the so far accumulated byte count. This matches GCCs behaviour as well: https://github.com/gcc-mirror/gcc/blob/bc8d6c60137f8bbf173b86ddf31b15d7ba2a33dd/gcc/config/i386/winnt.c#L203

Fixes https://github.com/llvm/llvm-project/issues/52782

[zero9178]

Friendly bump :)

[mstorsjo]

LGTM, the change seems sensible to me. But I’d prefer if you could hold off pushing it for another couple days, as others who might want to comment might not be following during the holiday season.

[majnemer]

I wonder if we should have different behavior for MSVC targets.

If I do:

class Incomplete;
extern "C" int __stdcall Fn(int, Incomplete, long long);
auto fnptr = &Fn;

MSVC generates:

EXTRN   _Fn@12:PROC

It appears that they skip over incomplete types.

Should the behavior for incomplete types depend on the target?

[zero9178]

In D116020#3218453, @majnemer wrote:

I wonder if we should have different behavior for MSVC targets.

If I do:

class Incomplete;
extern "C" int __stdcall Fn(int, Incomplete, long long);
auto fnptr = &Fn;

MSVC generates:

EXTRN   _Fn@12:PROC

It appears that they skip over incomplete types.

Should the behavior for incomplete types depend on the target?

That's actually a good point! I originally failed to get MSVC to generate a symbol for just the function declaration and thought it might just be irrelevant for MSVC. I didn't think of just using the function as you did lol. In the case of MinGW it always gets through the mangled as debug info is generated for the whole class, including just the method declaration.

I experimented a bit with your code and found a few interesting things:
clang actually refuses taking the address of a stdcall function if it has an incomplete type parameter:

clang-cl test.cpp test2.cpp --target=i686-pc-windows-msvc
test.cpp(3,15): error: parameter '' must have a complete type to use function 'Fn' with the stdcall calling convention
auto fnptr = &Fn;
              ^
test.cpp(1,7): note: forward declaration of 'Incomplete'
class Incomplete;
      ^
1 error generated.

Meanwhile cl will compile but fail to link with another object file that calls fnptr:

test.obj : error LNK2001: unresolved external symbol _Fn@12
  Hint on symbols that are defined and could potentially match:
    _Fn@16
test.exe : fatal error LNK1120: 1 unresolved externals

As both of them error out one way I am guessing it simply doesn't matter how the incomplete stdcall gets mangled. One cannot create calls to the function while the parameter type is incomplete and therefore no references to it are created.

How DWARF debuggers handle the "wrong" declarations in the debug info I have no clue about however. Given that GCC works the same however I am guessing gdb handles it correctly.

[rnk]

clang actually refuses taking the address of a stdcall function if it has an incomplete type parameter:

I was going to say, I thought I remembered fixing this problem, and I guess that's how I fixed it: with errors.

MSVC doesn't add stdcall mangling suffixes to non-extern-C symbols, so we don't have this kind of issue in MSVC mode.

I think the change is reasonable as is.

clang/test/CodeGen/pr52782-stdcall-func-decl.cpp

10

Please check for the declaration with the mangled name. In this case, we expect to see a @0 suffix.

Also, the NS_IMETHOD_ macro isn't necessary for the reduction, it can just be void __stdcall InitializeWithDrawTarget(NotNull).

[zero9178]

Update test according to reviewers comment

[rnk]

lgtm

clang/test/CodeGen/pr52782-stdcall-func-decl.cpp

10

I see, the @4 suffix is for this.

clang/lib/AST/Mangle.cpp

if (!Proto) {
     Out << '0';
     return;
   }
   assert(!Proto->isVariadic());
   unsigned ArgWords = 0;
   if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD))
     if (!MD->isStatic())
       ++ArgWords;
-  for (const auto &AT : Proto->param_types())
+  for (const auto &AT : Proto->param_types()) {
+    // If an argument type is incomplete there is no way to get its size to
+    // correctly encode into the mangling scheme.
+    // Follow GCCs behaviour by simply breaking out of the loop.
+    if (AT->isIncompleteType())
+      break;
     // Size should be aligned to pointer size.
     ArgWords +=
         llvm::alignTo(ASTContext.getTypeSize(AT), TI.getPointerWidth(0)) /
         TI.getPointerWidth(0);
+  }
   Out << ((TI.getPointerWidth(0) / 8) * ArgWords);
 }
 
 void MangleContext::mangleMSGuidDecl(const MSGuidDecl *GD, raw_ostream &Out) {
   // For now, follow the MSVC naming convention for GUID objects on all
   // targets.
   MSGuidDecl::Parts P = GD->getParts();
   Out << llvm::format("_GUID_%08" PRIx32 "_%04" PRIx32 "_%04" PRIx32 "_",

clang/test/CodeGen/pr52782-stdcall-func-decl.cpp

+// RUN: %clang_cc1 -triple i686-w64-windows-gnu -o - -emit-llvm -debug-info-kind=constructor %s
+
+#define NS_IMETHOD_(type) type __stdcall
+
+enum nsresult {};
+
+class NotNull;
+
+class nsICanvasRenderingContextInternal {
+  NS_IMETHOD_(nsresult) InitializeWithDrawTarget(NotNull);
+} nsTBaseHashSet;