dblaikie added subscribers: AndrewLitteken, bbn, JDevlieghere, jdoerfert, 
aprantl.
dblaikie added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to