[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Wenlei He via Phabricator via cfe-commits]

wenlei added a subscriber: lxfind.
wenlei added a comment.

In D93747#2475852 <https://reviews.llvm.org/D93747#2475852>, @dblaikie wrote:

> In D93747#2470504 <https://reviews.llvm.org/D93747#2470504>, @hoy wrote:
>
>> In D93747#2470387 <https://reviews.llvm.org/D93747#2470387>, @dblaikie wrote:
>>
>>> Please remove the clang test change - if this is an LLVM change with LLVM test coverage, that's enough. (we don't generally test every LLVM change from both LLVM and Clang)
>>
>> Sounds good.
>>
>>> I'd still be curious if you could look around to see whether there are other cases of function splitting/cloning/etc to see how they deal with updating the DISubprograms, to see if there's some prior art that should be used here.
>>
>> To the best of my knowledge, existing code does not change the linkage name field of a DISubprogram once created. You can create a new DISubprogram record with any linkage name you want but bear in mind how the debugger will consume the record. Looks like we are the first one to change existing record which will confuse the debugger.
>
> Sure enough - do any other transforms have similar requirements (of creating a record for something that looks like the same function but isn't quite) but take a different approach? Be good to know if other approaches were chosen/how/why they are applicable there but not here, etc. (conversely perhaps other passes worked around the issue in less than ideal ways and could benefit from using this new approach).
>
> Looks like some places that could use this functionality aren't quite there yet - 
> The Attributor has an internalizing feature (added in 87a85f3d57f55f5652ec44f77816c7c9457545fa <https://reviews.llvm.org/rG87a85f3d57f55f5652ec44f77816c7c9457545fa> ) that produces invalid IR (ends up with two !dbg attachments of the same DISubprogram) if the function it's internalizing has a DISubprogram - but if it succeeded it'd have the same problem that the linkage name on the DISubprogram wouldn't match the actual symbol/function name. (@jdoerfert @bbn).
> The IR Outliner (@AndrewLitteken ) seems to be only a few months old and appears to have no debug info handling - probably results in the same problem.
> The Partial Inliner does clone a function into a new name & so would have an invalid DISubprogram, though it only uses the clone for inlining (this probably then goes on to produce the desired debug info, where the inlining appears to come from the original function)
> ThinLTO does some function cloning within a module for aliases, but it then renames the clone to the aliasees name so I think the name works out to match again.

Another place where mismatch between linkage name and DISubprogram happens is the cloning for coroutines (.resume, .destroy and cleanup funclets). We found that out again through different kind of AutoFDO profile fidelity issue (cc: @lxfind).

> If this is an invariant, that the linkage name on the DISubprogram should match the actual llvm::Function name (@aprantl @JDevlieghere - verifier check, perhaps?) - it'd be nice to make that more reliable, either by removing the name and relying on the llvm::Function name (perhaps with a boolean on the DISubprogram as to whether the linkage name should be emitted or not - I think currently Clang's IRGen makes choices about which DISubprograms will get linkage names) or a verifier and utilities to keep them in sync.

Agreed, clarity on the rules and sanity check built into verifier would be beneficial.

> But I'll leave that alone for now/for this review, unless someone else wants to chime in on it.
>
> In D93747#2470178 <https://reviews.llvm.org/D93747#2470178>, @tmsriram wrote:
>
>> In D93747#2469556 <https://reviews.llvm.org/D93747#2469556>, @hoy wrote:
>>
>>>> In D93656 <https://reviews.llvm.org/D93656>, @dblaikie wrote:
>>>> Though the C case is interesting - it means you'll end up with C functions with the same DWARF 'name' but different linkage name. I don't know what that'll do to DWARF consumers - I guess they'll probably be OK-ish with it, as much as they are with C++ overloading. I think there are some cases of C name mangling so it's probably supported/OK-ish with DWARF Consumers. Wouldn't hurt for you to take a look/see what happens in that case with a debugger like gdb/check other cases of C name mangling to see what DWARF they end up creating (with both GCC and Clang).
>>>
>>> I did a quick experiment with C name managing with GCC and -flto. It turns out the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never set for C programs. If set, the gdb debugger will use that field to match the user input and set breakpoints. Therefore, giving `DW_AT_linkage_name` a uniquefied name prevents the debugger from setting a breakpoint based on source names unless the user specifies a decorated name.
>>>
>>> Hence, this approach sounds like a workaround for us when the profile quality matters more than debugging experience. I'm inclined to have it under a switch. What do you think?
>>
>> Just a thought, we could always check if rawLinkageName is set and only set it when it is not null.  That seems safe without needing the option. Not a strong opinion.
>
> Was this thread concluded? Doesn't look like any check was added?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747