tmsriram added a comment. In D93747#2482726 <https://reviews.llvm.org/D93747#2482726>, @hoy wrote:
> In D93747#2482519 <https://reviews.llvm.org/D93747#2482519>, @tmsriram wrote: > >> In D93747#2481494 <https://reviews.llvm.org/D93747#2481494>, @dblaikie wrote: >> >>> In D93747#2481383 <https://reviews.llvm.org/D93747#2481383>, @hoy wrote: >>> >>>> In D93747#2481304 <https://reviews.llvm.org/D93747#2481304>, @tmsriram >>>> wrote: >>>> >>>>> In D93747#2481223 <https://reviews.llvm.org/D93747#2481223>, @tmsriram >>>>> wrote: >>>>> >>>>>> In D93747#2481163 <https://reviews.llvm.org/D93747#2481163>, @dblaikie >>>>>> wrote: >>>>>> >>>>>>> In D93747#2481095 <https://reviews.llvm.org/D93747#2481095>, @hoy wrote: >>>>>>> >>>>>>>> In D93747#2481073 <https://reviews.llvm.org/D93747#2481073>, @dblaikie >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Suggesting that gdb isn't using the DW_AT_name at all for "break >>>>>>>>> <function name>" but instead demangling and stripping off the extras >>>>>>>>> from the linkage name, and since it can't demangle this uniquified >>>>>>>>> name (unlike the mangled name used when using the overloadable >>>>>>>>> attribute) that degrades the debugger user experience? I'd have to >>>>>>>>> test that in more detail/with some hand-hacked DWARF. >>>>>>>> >>>>>>>> Yes, I think in your case the linage name can be demangled by the >>>>>>>> debugger. In my previous experiment, the uniquefied names could not be >>>>>>>> demangled therefore I was not able to breakpoint. >>>>>>> >>>>>>> Ah, did some more testing. Seems it's because it isn't a classically >>>>>>> mangled name. >>>>> >>>>> The simplest fix I can think of is to emit the base 10 number of the >>>>> md5hash. That would preserve all the existing uniqueness and be >>>>> demangler friendly. @hoy @dblaikie WDYT? >>>> >>>> I think using the base 10 form of md5hash is a smart workaround. Thanks >>>> for the suggestion. >>> >>> Sure - wouldn't mind having some wording from the Itanium ABI/mangling >>> rules that explain/corroborate what we've seen from testing to ensure we >>> are using it correctly and there aren't any other ways that might be more >>> suitable, or lurking gotchas we haven't tested. >> >> Yes, makes sense. Also, like @dblaikie pointed out in D94154 >> <https://reviews.llvm.org/D94154> it is now important that we don't generate >> the DW_AT_linkage_name for C style names using this suffix as it will not >> demangle. I think this makes it important that we only update this field if >> it already exists. > > Yes, it makes sense to do the renaming only if the linkage name preexists to > not break the debuggability. Unfortunately we won't be able to support C with > this patch. There is nothing needed to support C right? overloadable attribute will mangle with _Z so it will work as expected? What did I miss? >>>>>>> It might be possible for this uniquifying step to check if the name is >>>>>>> mangled (does it start with "_Z") and if it isn't mangled, it could >>>>>>> mangle it (check the length of the name, then "_Z<length><name>v") and >>>>>>> add the unique suffix. That looks to me like it preserves the original >>>>>>> debugging behavior? >>>>>>> >>>>>>> (has the problem that we don't actually know the mangling scheme at >>>>>>> this point - we do know it up in clang (where it might be Itanium >>>>>>> mangling or Microsoft mangling), might point again to the possibility >>>>>>> this feature is more suitable to implement in the frontend rather than >>>>>>> a middle end pass. And also the 'v' in the mangling is 'void return >>>>>>> type', which is a lie - not sure if we could do something better there) >>>> >>>> Doing name unification in the frontend sounds like the ultimate solution >>>> and since the frontend has all the knowledge about the name mangler. I >>>> think for now we can go with the solution @tmsriram suggested, i.e., using >>>> the base 10 form of md5 hash. >>> >>> Any general idea of how long "for now" would be? It doesn't seem like >>> putting this off is going to make things especially better & seems like >>> more bug fixes/changes/etc are being built around the solution as it is at >>> the moment. So I'd be hesitant to put off this kind of restructuring too >>> long. > > I'm not sure. It looks like implementation in the front end has more > complexity as discussed in the other thread D94154 > <https://reviews.llvm.org/D94154>. > >>>>>>> I think it's pretty important that we don't break tradeoff >>>>>>> debuggability for profile accuracy. It'll make for a difficult tradeoff >>>>>>> for our/any users. >>>>>> >>>>>> Agreed, I didn't expect this. >>>>>> >>>>>>> (side note: using the original source file name seems problematic - I >>>>>>> know in google at least, the same source file is sometimes built into >>>>>>> more than one library/form with different preprocessor defines, so this >>>>>>> may not produce as unique a name as you are expecting?) >>>>>> >>>>>> It was a best effort and I think the hashing can be improved by using >>>>>> more signals other than just the module name if needed. For hard data >>>>>> though, this does significantly improve performance which clearly comes >>>>>> from better profile attribution so it does something. >>> >>> I'm OK with the idea that this helped the situation - but it doesn't seem >>> very principled/rigorous. Rather than a sliding scale of "better" I think >>> it'd be worth considering in more detail what would be required to make >>> this correct. >>> >>> Using the object file name may be more robust - also not perfect for all >>> build systems (one could imagine a distributed build system that /might/ be >>> able to get away with having the distributed builders always build a file >>> named x.o - only to have the distribution system rename the file to its >>> canonical name on the main machine before linking occurs - but I think >>> that's not how most distributed build systems work, and most build systems >>> provide a unique object file name across a build?) but maybe moreso than >>> using the input file name? (at least I think for google/blaze the object >>> filename is genuinely unique) >> >> So we are using the full path name of the source. We could also combine the >> object file name into the hash to make it more robust. I can work on this >> patch independently if that is alright. 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
