[PATCH] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Thanks for taking a look, @rjmccall - I really appreciate it!

Sorry I'm doing a bad job describing (admittedly I'm pretty confused about all this, which is most of the issue) the issue - I'll try to clarify as best I can.

In D98799#2781200 <https://reviews.llvm.org/D98799#2781200>, @rjmccall wrote:

> In D98799#2761684 <https://reviews.llvm.org/D98799#2761684>, @dblaikie wrote:
>
>> OK - poked around a bit more to better understand this, so attempting to summarize my current understanding (excuse the repetition from previous parts of this thread) and results.
>>
>> - `__attribute__((overloadable))` can/does mangle K&R C style declarations <https://reviews.llvm.org/D98799#2638997>
>> - with this patch (currently committed), `-funique-internal-linkage-names` does not mangle K&R C style declarations (see `bar` in the included test case, unmangled)
>> - I'd like to avoid that divergence if possible
>> - Changing the debug info code to be more generous with names it mangles (by using `FD->getType()->getAs<FunctionProtoType>()` rather than `hasPrototype()`) causes problems
>>   - Specifically: Objective C blocks (which have a `FunctionProtoType` type, but `!hasPrototype` it seems) are missing parameter info so this <https://github.com/llvm-mirror/clang/blob/aa231e4be75ac4759c236b755c57876f76e3cf05/lib/AST/ItaniumMangle.cpp#L2908> call crashes
>>     - There doesn't seem to be any way to test for this property of the `FunctionDecl` that I can see - where it has a type, but doesn't have parameter info
>>
>> Trying to pull in some folks who might know what's going on here/be able to suggest a way to split these cases if needed, or fix the block `FunctionDecl`s to have param info. @rjmccall @JDevlieghere - I'd really appreciate some help here.
>
> Unlike lambdas, `BlockDecl` is not a subclass of `FunctionDecl`, so I'm not quite sure what you're asking — there shouldn't be a way for that line to crash on a `BlockDecl` because `FD` should be null.
>
> Not sure what you mean by block `FunctionDecl` — a block doesn't make a `FunctionDecl`.  If it's useful for `BlockDecl` to return

Did this ^ get cut off in some way ("If it's useful for `BlockDecl` to return" ... something?).

Anyway - so this crash only seems to happen in Objective C++ tests <https://reviews.llvm.org/D98799#2757976>. It's not the block itself, but I'm guessing some implementation function, specifically:

  FunctionDecl 0xf921248 <<invalid sloc>> <invalid sloc> __Block_byref_object_copy_ 'void (void *, void *)' static <<<NULL params x 2>>>

This `FunctionDecl`'s type is a `FunctionProtoType`, but doesn't have any parameter type info, which seems weird. Maybe that's the bug - perhaps whatever creates this should be adding type info like for other `FunctionProtoType`d `FunctionDecl`s?

Ah, here it is: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBlocks.cpp#L1958 - that creates this seemingly weird FunctionDecl that has a prototype type, but if you try to query the type of its parameters, it'll crash.

Other `FunctionDecl`s get their `ParmVarDecl`s in places like this: https://github.com/llvm/llvm-project/blob/7daa18215905c831e130c7542f17619e9d936dfc/clang/lib/Sema/SemaDecl.cpp#L9478 and look like this:

  FunctionDecl 0xf923bb0 <test.cpp:1:1, col:42> col:42 go 'int (int)' static
  `-ParmVarDecl 0xf923ae8 <col:45, col:49> col:49 a 'int'

Should the synthetic `FunctionDecl` created by CGBlocks.cpp be changed to be more fully featured and have valid `ParmVarDecl`s?

[aside: Looks like the bug here that I'm interested in sort of already exists apart from the unique linkage name case here - we don't add the mangled name of `__attribute__((overloadable))` to DWARF even though we should, and if I try to fix that the obvious way (testing `FD->getType()->getAs<FunctionProtoType>()` instead of `FD->hasPrototype()` in `collectFunctionDeclProps` then it crashes the ObjC++ block tests for the above reason]


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98799/new/

https://reviews.llvm.org/D98799