This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by zero9178 on Dec 20 2021, 12:40 AM.

Details

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

Diff Detail

Unit TestsFailed

Event Timeline

zero9178 requested review of this revision.Dec 20 2021, 12:40 AM
zero9178 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 20 2021, 12:40 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Friendly bump :)

mstorsjo accepted this revision.Jan 3 2022, 2:50 PM

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.

This revision is now accepted and ready to land.Jan 3 2022, 2:50 PM

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?

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 added a comment.Jan 4 2022, 2:28 PM

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 updated this revision to Diff 397524.Jan 5 2022, 4:26 AM

Update test according to reviewers comment

rnk accepted this revision.Jan 5 2022, 7:40 AM

lgtm

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

I see, the @4 suffix is for this.

This revision was landed with ongoing or failed builds.Jan 5 2022, 8:58 AM
This revision was automatically updated to reflect the committed changes.