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

Sat Jan 2 13:44:09 PST 2021

hoy 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.
>
> 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.
>
> 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?

Thanks for the detailed analysis! Moving forward I would like to see a more clear contract about debug linkage names between the compiler and debugger as a guideline to code cloning work. For this patch, I'm adding a switch for it with a default value "on" since AutoFDO and propeller are the primary consumers of the work here and they mostly care about profile quality more than debugging experience. But correct me if I'm wrong and I'll flip the default value.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747