[PATCH] D94154: Unique Internal Linkage Name suffixes must be demangler friendly

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

In D94154#2485197 <https://reviews.llvm.org/D94154#2485197>, @dblaikie wrote:

> In D94154#2482425 <https://reviews.llvm.org/D94154#2482425>, @rnk wrote:
>
>> In D94154#2481491 <https://reviews.llvm.org/D94154#2481491>, @dblaikie wrote:
>>
>>> This is only demangler friendly if the name is already an itanium mangled name, right? (ie: the name starts with _Z) So it wouldn't work for C code?
>>
>> I'm pretty sure most demangling tools such as c++filt check for a leading prefix of `_+Z` before demangling, so I don't think there are any concerns for C names. A reasonable demangler would pass them through as is.
>
> Yep - but passing them through unmodified was causing problems for gdb which was demangling the mangled names as they appear in the DWARF and using the unmangled name to figure out how to do name lookup. So if the symbol wasn't getting unmangled you wouldn't be able to "break f1" instead you'd have to "break f1.__part.1" which would be an unfortunate regression in debuggability.
>
> But it seems like that only applies if a mangled name is present in the DWARF at all - if no mangled name is present, and the debug info just gives the pretty name it works OK. Bit weird to have no record of the real symbol name in the DWARF, but so far it doesn't seem to cause any problems? So I'm OK-ish with this.
>
>>> Do you have plans to fix this more generally? (I think to fix it more generally, you might need to move this feature up to clang and have clang mangle the name then add the suffix (at least for itanium mangling - not sure if windows mangling supports arbitrary suffixes like this, @rnk might know) - that way for C functions you can force/enable the mangling as is done for __attribute__((overloadable)) and others (I think __attribute__((enable_if)) also causes mangling of C functions))
>>
>> That was Sriram's original idea: have the mangler do it. I reviewed the code. Modifying the mangler was fairly complicated, so I suggested doing it in a pass. The original pass also operated by appending a suffix after mangling, it just happened earlier. There isn't a good way to "embed" this uniquification into the Itanium mangling scheme, so far as I am aware.
>
> Oh, I was still in favor of adding it after the mangling (the current "mangled.__part.number") but I thought it may be necessary to force mangling on C functions before adding the suffix, if they needed to be demangleable for debug info purposes.
>
>> That all said, I don't see any reason to block this decimilization change. It's limited in scope and an obvious improvement.
>
> My concern was that it was layering more workarounds on a patch series that might be going in the wrong direction overall.
>
> Anyway, sounds like, if I'm understanding/tested correctly, that the issue with unmangleability isn't about the symbol name itself but the DW_AT_linkage_name in the DWARF, so it seems like if that isn't present then there's no issue if the real symbol name can't be unmangled/back to the simple name. (but there's some reason the DW_AT_linkage_name, if present, must match the symbol name? (so it can't be the original unsuffixed mangled name))

One reason that DW_AT_linkage_name should be consistent with the real linkage name is to favor AutoFDO. DW_AT_linkage_name is used to generate profiles for inlined functions.


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

https://reviews.llvm.org/D94154