tmsriram added a comment. 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: >>> >>>> In D93747#2481053 <https://reviews.llvm.org/D93747#2481053>, @tmsriram >>>> wrote: >>>> >>>>> In D93747#2480442 <https://reviews.llvm.org/D93747#2480442>, @dblaikie >>>>> wrote: >>>>> >>>>>> @tmsriram - any ideas what your case/example was like that might've >>>>>> caused degraded debugging experience? Would be good to understand if >>>>>> we're producing some bad DWARF with this patch/if there might be some >>>>>> way to avoid it (as it seems like gdb can handle mangled >>>>>> names/overloading in C in this example I've tried) >>>>> >>>>> I haven't seen anything that caused degraded debugging experience. I am >>>>> interested in this as we do look at this field for the purposes of >>>>> profile attribtution for calls that are inlined. My comment was that we >>>>> don't need to create this if it didn't already exist. I am not fully >>>>> aware of what would happen if we did it all the time. >>>> >>>> Ah, sorry, I got confused as to who's comment I was reading. I see it was >>>> @hoy who said: >>>> >>>>> 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. >>>> >>>> So I'm still a bit confused. My test doesn't seem to demonstrate the issue >>>> with setting a linkage name preventing the debugger from setting a >>>> breakpoint based on the source name? >>>> >>>> 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? > Yep, that's the issue > > $ c++filt _Z3foov.__uniq > foo() [clone .__uniq] > > $ c++filt _Z3foov.__uniq.123 > foo() [clone .__uniq.123] > > $ c++filt._Z3foov.__uniq.123abc > _Z3foov.__uniq.123abc > > The demangler does not understand a mix of numeric and alpha-numeric. It can > be either but not both and md5hashes contain both. So we might have to > process the hash string or do something different to keep it demangler > friendly. > >> 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) >> >> 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. 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
