[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names
Hongtao Yu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 5 18:12:13 PST 2021
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 at blaikie-linux2:~/dev/scratch$ clang-tot test.c -g
>> blaikie at 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 (git at 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
More information about the cfe-commits
mailing list