hoy added a comment. In D93747#2481053 <https://reviews.llvm.org/D93747#2481053>, @tmsriram wrote:
> 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. Thanks for sharing your experience. I was wondering If we only replace debug linkage names when they pre-exist, then it may not work for most of the C programs where the debug linkage names are not set by default. 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
