tmsriram added a comment. In D93747#2480442 <https://reviews.llvm.org/D93747#2480442>, @dblaikie wrote:
>>> 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. > > Presumably people are going to want to debug these binaries - I'm not sure > it's a "one or the other" situation as it sounds like @tmsriram was > suggesting by only modifying the linkage name when it's already set this > might produce a better debugging experience, if I'm following the discussion > correctly? > > But I'm pretty sure there are places where C supports name mangling, so I > wouldn't mind understanding the gdb behavior in more detail - perhaps there's > a way to make it work better. > > Using Clang's __attribute__((overloadable)) support, I was able to produce a > C program with mangled names, with DW_AT_linkage_names on those functions, > like this: > > $ cat test.c > void __attribute__((overloadable)) f(int i) { > } > void __attribute__((overloadable)) f(long l) { > } > int main() { > f(3); > f(5l); > } > blaikie@blaikie-linux2:~/dev/scratch$ clang-tot test.c -g > blaikie@blaikie-linux2:~/dev/scratch$ llvm-dwarfdump-tot a.out > a.out: file format elf64-x86-64 > > .debug_info contents: > 0x00000000: Compile Unit: length = 0x0000009e, format = DWARF32, version = > 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x000000a2) > > 0x0000000b: DW_TAG_compile_unit > DW_AT_producer ("clang version 12.0.0 > (g...@github.com:llvm/llvm-project.git > 20013e0940dea465a173c3c7ce60f0e419242ef8)") > DW_AT_language (DW_LANG_C99) > DW_AT_name ("test.c") > DW_AT_stmt_list (0x00000000) > DW_AT_comp_dir > ("/usr/local/google/home/blaikie/dev/scratch") > DW_AT_low_pc (0x0000000000401110) > DW_AT_high_pc (0x000000000040114c) > > 0x0000002a: DW_TAG_subprogram > DW_AT_low_pc (0x0000000000401110) > DW_AT_high_pc (0x0000000000401119) > DW_AT_frame_base (DW_OP_reg6 RBP) > DW_AT_linkage_name ("_Z1fi") > DW_AT_name ("f") > DW_AT_decl_file > ("/usr/local/google/home/blaikie/dev/scratch/test.c") > DW_AT_decl_line (1) > DW_AT_prototyped (true) > DW_AT_external (true) > > 0x00000043: DW_TAG_formal_parameter > DW_AT_location (DW_OP_fbreg -4) > DW_AT_name ("i") > DW_AT_decl_file > ("/usr/local/google/home/blaikie/dev/scratch/test.c") > DW_AT_decl_line (1) > DW_AT_type (0x00000093 "int") > > 0x00000051: NULL > > 0x00000052: DW_TAG_subprogram > DW_AT_low_pc (0x0000000000401120) > DW_AT_high_pc (0x000000000040112a) > DW_AT_frame_base (DW_OP_reg6 RBP) > DW_AT_linkage_name ("_Z1fl") > DW_AT_name ("f") > DW_AT_decl_file > ("/usr/local/google/home/blaikie/dev/scratch/test.c") > DW_AT_decl_line (3) > DW_AT_prototyped (true) > DW_AT_external (true) > > 0x0000006b: DW_TAG_formal_parameter > DW_AT_location (DW_OP_fbreg -8) > DW_AT_name ("l") > DW_AT_decl_file > ("/usr/local/google/home/blaikie/dev/scratch/test.c") > DW_AT_decl_line (3) > DW_AT_type (0x0000009a "long int") > > 0x00000079: NULL > > 0x0000007a: DW_TAG_subprogram > DW_AT_low_pc (0x0000000000401130) > DW_AT_high_pc (0x000000000040114c) > DW_AT_frame_base (DW_OP_reg6 RBP) > DW_AT_name ("main") > DW_AT_decl_file > ("/usr/local/google/home/blaikie/dev/scratch/test.c") > DW_AT_decl_line (5) > DW_AT_type (0x00000093 "int") > DW_AT_external (true) > > 0x00000093: DW_TAG_base_type > DW_AT_name ("int") > DW_AT_encoding (DW_ATE_signed) > DW_AT_byte_size (0x04) > > 0x0000009a: DW_TAG_base_type > DW_AT_name ("long int") > DW_AT_encoding (DW_ATE_signed) > DW_AT_byte_size (0x08) > > 0x000000a1: NULL > $ gdb ./a.out > GNU gdb (GDB) 10.0-gg5 > Copyright (C) 2020 Free Software Foundation, Inc. > License GPLv3+: GNU GPL version 3 or later > <http://gnu.org/licenses/gpl.html> > This is free software: you are free to change and redistribute it. > There is NO WARRANTY, to the extent permitted by law. > Type "show copying" and "show warranty" for details. > This GDB was configured as "x86_64-grtev4-linux-gnu". > Type "show configuration" for configuration details. > > <http://go/gdb-home FAQ: http://go/gdb-faq Email: c-toolchain-team> > Find the GDB manual and other documentation resources online at: > <http://www.gnu.org/software/gdb/documentation/>. > > For help, type "help". > Type "apropos word" to search for commands related to "word". > Reading symbols from ./a.out... > Unable to determine compiler version. > Skipping loading of libstdc++ pretty-printers for now. > Loading libc++ pretty-printers. > Non-google3 binary detected. > (gdb) b f(int) > Breakpoint 1 at 0x401117: file test.c, line 2. > (gdb) b f(long) > Breakpoint 2 at 0x401128: file test.c, line 4. > (gdb) del 1 > (gdb) r > Starting program: /usr/local/google/home/blaikie/dev/scratch/a.out > > Breakpoint 2, _Z1fl (l=5) at test.c:4 > 4 } > (gdb) c > Continuing. > [Inferior 1 (process 497141) exited normally] > quit) > Aborted > > @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. 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
