Page MenuHomePhabricator

Rename debug linkage name with -funique-internal-linkage-names
ClosedPublic

Authored by hoy on Dec 22 2020, 11:18 PM.

Details

Summary

Functions that are renamed under -funique-internal-linkage-names have their debug linkage name updated as well.

Diff Detail

Event Timeline

hoy created this revision.Dec 22 2020, 11:18 PM
hoy requested review of this revision.Dec 22 2020, 11:18 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 22 2020, 11:18 PM
hoy added a comment.Dec 22 2020, 11:37 PM

In 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?

In D93747#2469556, @hoy wrote:

In 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.

hoy added a comment.Dec 23 2020, 9:40 AM
In D93747#2469556, @hoy wrote:

In 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.

It seems that the demangler of the debugger is not able to handle an uniquefied name, even if the debug record originally comes with a linkage name.

In D93747#2470189, @hoy wrote:
In D93747#2469556, @hoy wrote:

In 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.

It seems that the demangler of the debugger is not able to handle an uniquefied name, even if the debug record originally comes with a linkage name.

Yep, the demangler (which can be checked with c++filt) is very restricted on what comes after the "." It can be either numbers or characters but not a mix of the two. I guess we can enhance it.

Please remove the clang test change - if this is an LLVM change with LLVM test coverage, that's enough. (we don't generally test every LLVM change from both LLVM and Clang)

I'd still be curious if you could look around to see whether there are other cases of function splitting/cloning/etc to see how they deal with updating the DISubprograms, to see if there's some prior art that should be used here.

hoy added a comment.Dec 23 2020, 1:19 PM

Please remove the clang test change - if this is an LLVM change with LLVM test coverage, that's enough. (we don't generally test every LLVM change from both LLVM and Clang)

Sounds good.

I'd still be curious if you could look around to see whether there are other cases of function splitting/cloning/etc to see how they deal with updating the DISubprograms, to see if there's some prior art that should be used here.

To the best of my knowledge, existing code does not change the linkage name field of a DISubprogram once created. You can create a new DISubprogram record with any linkage name you want but bear in mind how the debugger will consume the record. Looks like we are the first one to change existing record which will confuse the debugger.

hoy updated this revision to Diff 313607.Dec 23 2020, 1:20 PM

Undoing changes to the clang test.

This change looks good to me but I would like @rnk to sign off. My opinion is that the change is simple enough.

In D93747#2470504, @hoy wrote:

Please remove the clang test change - if this is an LLVM change with LLVM test coverage, that's enough. (we don't generally test every LLVM change from both LLVM and Clang)

Sounds good.

I'd still be curious if you could look around to see whether there are other cases of function splitting/cloning/etc to see how they deal with updating the DISubprograms, to see if there's some prior art that should be used here.

To the best of my knowledge, existing code does not change the linkage name field of a DISubprogram once created. You can create a new DISubprogram record with any linkage name you want but bear in mind how the debugger will consume the record. Looks like we are the first one to change existing record which will confuse the debugger.

Sure enough - do any other transforms have similar requirements (of creating a record for something that looks like the same function but isn't quite) but take a different approach? Be good to know if other approaches were chosen/how/why they are applicable there but not here, etc. (conversely perhaps other passes worked around the issue in less than ideal ways and could benefit from using this new approach).

Looks like some places that could use this functionality aren't quite there yet -
The Attributor has an internalizing feature (added in 87a85f3d57f55f5652ec44f77816c7c9457545fa ) that produces invalid IR (ends up with two !dbg attachments of the same DISubprogram) if the function it's internalizing has a DISubprogram - but if it succeeded it'd have the same problem that the linkage name on the DISubprogram wouldn't match the actual symbol/function name. (@jdoerfert @bbn).
The IR Outliner (@AndrewLitteken ) seems to be only a few months old and appears to have no debug info handling - probably results in the same problem.
The Partial Inliner does clone a function into a new name & so would have an invalid DISubprogram, though it only uses the clone for inlining (this probably then goes on to produce the desired debug info, where the inlining appears to come from the original function)
ThinLTO does some function cloning within a module for aliases, but it then renames the clone to the aliasees name so I think the name works out to match again.

If this is an invariant, that the linkage name on the DISubprogram should match the actual llvm::Function name (@aprantl @JDevlieghere - verifier check, perhaps?) - it'd be nice to make that more reliable, either by removing the name and relying on the llvm::Function name (perhaps with a boolean on the DISubprogram as to whether the linkage name should be emitted or not - I think currently Clang's IRGen makes choices about which DISubprograms will get linkage names) or a verifier and utilities to keep them in sync.

But I'll leave that alone for now/for this review, unless someone else wants to chime in on it.

In D93747#2469556, @hoy wrote:

In 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?

hoy updated this revision to Diff 314259.Sat, Jan 2, 1:36 PM

Adding a switch.

hoy added a comment.Sat, Jan 2, 1:44 PM
In D93747#2470504, @hoy wrote:

Please remove the clang test change - if this is an LLVM change with LLVM test coverage, that's enough. (we don't generally test every LLVM change from both LLVM and Clang)

Sounds good.

I'd still be curious if you could look around to see whether there are other cases of function splitting/cloning/etc to see how they deal with updating the DISubprograms, to see if there's some prior art that should be used here.

To the best of my knowledge, existing code does not change the linkage name field of a DISubprogram once created. You can create a new DISubprogram record with any linkage name you want but bear in mind how the debugger will consume the record. Looks like we are the first one to change existing record which will confuse the debugger.

Sure enough - do any other transforms have similar requirements (of creating a record for something that looks like the same function but isn't quite) but take a different approach? Be good to know if other approaches were chosen/how/why they are applicable there but not here, etc. (conversely perhaps other passes worked around the issue in less than ideal ways and could benefit from using this new approach).

Looks like some places that could use this functionality aren't quite there yet -
The Attributor has an internalizing feature (added in 87a85f3d57f55f5652ec44f77816c7c9457545fa ) that produces invalid IR (ends up with two !dbg attachments of the same DISubprogram) if the function it's internalizing has a DISubprogram - but if it succeeded it'd have the same problem that the linkage name on the DISubprogram wouldn't match the actual symbol/function name. (@jdoerfert @bbn).
The IR Outliner (@AndrewLitteken ) seems to be only a few months old and appears to have no debug info handling - probably results in the same problem.
The Partial Inliner does clone a function into a new name & so would have an invalid DISubprogram, though it only uses the clone for inlining (this probably then goes on to produce the desired debug info, where the inlining appears to come from the original function)
ThinLTO does some function cloning within a module for aliases, but it then renames the clone to the aliasees name so I think the name works out to match again.

If this is an invariant, that the linkage name on the DISubprogram should match the actual llvm::Function name (@aprantl @JDevlieghere - verifier check, perhaps?) - it'd be nice to make that more reliable, either by removing the name and relying on the llvm::Function name (perhaps with a boolean on the DISubprogram as to whether the linkage name should be emitted or not - I think currently Clang's IRGen makes choices about which DISubprograms will get linkage names) or a verifier and utilities to keep them in sync.

But I'll leave that alone for now/for this review, unless someone else wants to chime in on it.

In D93747#2469556, @hoy wrote:

In 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.

hoy updated this revision to Diff 314393.Mon, Jan 4, 9:09 AM

Replacing linkage names for debug declaration as well.

In D93747#2469556, @hoy wrote:

In 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 (git@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)

In D93747#2469556, @hoy wrote:

In 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 (git@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.

hoy added a comment.Tue, Jan 5, 6:12 PM
In D93747#2469556, @hoy wrote:

In 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 (git@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.

@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.

hoy added a comment.Tue, Jan 5, 6:23 PM

@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.

In D93747#2481095, @hoy 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.

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.

(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?)

In D93747#2481095, @hoy 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.

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.

In D93747#2481095, @hoy 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.

hoy added a comment.Tue, Jan 5, 10:31 PM
In D93747#2481095, @hoy 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?

I think using the base 10 form of md5hash is a smart workaround. Thanks for the suggestion.

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)

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.

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.

In D93747#2481383, @hoy wrote:
In D93747#2481095, @hoy 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.

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 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)

In D93747#2481383, @hoy wrote:
In D93747#2481095, @hoy 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 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.

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 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.

hoy added a comment.Wed, Jan 6, 11:55 AM
In D93747#2481383, @hoy wrote:
In D93747#2481095, @hoy 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 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.

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.

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.

hoy updated this revision to Diff 314955.Wed, Jan 6, 11:56 AM

Renaming debug linkage name if it preexisits.

In D93747#2482726, @hoy wrote:
In D93747#2481383, @hoy wrote:
In D93747#2481095, @hoy 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 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.

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.

hoy added a comment.EditedWed, Jan 6, 11:59 AM
In D93747#2482726, @hoy wrote:
In D93747#2481383, @hoy wrote:
In D93747#2481095, @hoy 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 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?

I mean from the AutoFDO point of view, C programs with static functions are not getting the unique debug name support. But yes if we annotate them with the overloadable attribute.

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.

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.

dblaikie accepted this revision.Fri, Jan 8, 9:40 PM

Seems alright to me - I think we've hashed out the deeper issues (missing opportunity for C functions which could/should be addressed by moving the implementation to the frontend, where those C functions can be mangled and then use linkageName to give them the same AutoFDO opportunities as C++ functions) here and elsewhere - but for what it is, the patch makes sense. I'd probably say drop the flag - " check if rawLinkageName is set and only set it when it is not null. " was implemented and seems that addressed the debug info issue without an awkward tradeoff between AutoFDO fidelity and debugging fidelity, so there doesn't seem to be a need to be able to configure this.

This revision is now accepted and ready to land.Fri, Jan 8, 9:40 PM

Seems alright to me - I think we've hashed out the deeper issues (missing opportunity for C functions which could/should be addressed by moving the implementation to the frontend, where those C functions can be mangled and then use linkageName to give them the same AutoFDO opportunities as C++ functions) here and elsewhere - but for what it is, the patch makes sense. I'd probably say drop the flag - " check if rawLinkageName is set and only set it when it is not null. " was implemented and seems that addressed the debug info issue without an awkward tradeoff between AutoFDO fidelity and debugging fidelity, so there doesn't seem to be a need to be able to configure this.

Here is a suggestion for a plan forward. Let's get these patches along with D94154 in. No correctness issues but a missed opportunity. I will work with @rnk and @dblaikie and send out a patch where I move the uniqueification to clang? That patch will also do linkage name for C functions with mangled name when uniqueification is needed. Does that sound reasonable? As for timeline, I can do this in two weeks.

Seems alright to me - I think we've hashed out the deeper issues (missing opportunity for C functions which could/should be addressed by moving the implementation to the frontend, where those C functions can be mangled and then use linkageName to give them the same AutoFDO opportunities as C++ functions) here and elsewhere - but for what it is, the patch makes sense. I'd probably say drop the flag - " check if rawLinkageName is set and only set it when it is not null. " was implemented and seems that addressed the debug info issue without an awkward tradeoff between AutoFDO fidelity and debugging fidelity, so there doesn't seem to be a need to be able to configure this.

Here is a suggestion for a plan forward. Let's get these patches along with D94154 in. No correctness issues but a missed opportunity. I will work with @rnk and @dblaikie and send out a patch where I move the uniqueification to clang? That patch will also do linkage name for C functions with mangled name when uniqueification is needed. Does that sound reasonable? As for timeline, I can do this in two weeks.

Sure sure - not urgent, just so long as it doesn't get lost.

hoy added a comment.Fri, Jan 8, 10:58 PM

Seems alright to me - I think we've hashed out the deeper issues (missing opportunity for C functions which could/should be addressed by moving the implementation to the frontend, where those C functions can be mangled and then use linkageName to give them the same AutoFDO opportunities as C++ functions) here and elsewhere - but for what it is, the patch makes sense. I'd probably say drop the flag - " check if rawLinkageName is set and only set it when it is not null. " was implemented and seems that addressed the debug info issue without an awkward tradeoff between AutoFDO fidelity and debugging fidelity, so there doesn't seem to be a need to be able to configure this.

Here is a suggestion for a plan forward. Let's get these patches along with D94154 in. No correctness issues but a missed opportunity. I will work with @rnk and @dblaikie and send out a patch where I move the uniqueification to clang? That patch will also do linkage name for C functions with mangled name when uniqueification is needed. Does that sound reasonable? As for timeline, I can do this in two weeks.

Sure sure - not urgent, just so long as it doesn't get lost.

Sounds good to me, thanks for all the in-depth discussions!

hoy updated this revision to Diff 315572.Fri, Jan 8, 11:15 PM

Removing the UniqueDebugLinakgeNames switch.

hoy updated this revision to Diff 315908.Mon, Jan 11, 12:29 PM

Rebasing.

wenlei added a subscriber: lxfind.Wed, Jan 20, 9:02 AM
In D93747#2470504, @hoy wrote:

Please remove the clang test change - if this is an LLVM change with LLVM test coverage, that's enough. (we don't generally test every LLVM change from both LLVM and Clang)

Sounds good.

I'd still be curious if you could look around to see whether there are other cases of function splitting/cloning/etc to see how they deal with updating the DISubprograms, to see if there's some prior art that should be used here.

To the best of my knowledge, existing code does not change the linkage name field of a DISubprogram once created. You can create a new DISubprogram record with any linkage name you want but bear in mind how the debugger will consume the record. Looks like we are the first one to change existing record which will confuse the debugger.

Sure enough - do any other transforms have similar requirements (of creating a record for something that looks like the same function but isn't quite) but take a different approach? Be good to know if other approaches were chosen/how/why they are applicable there but not here, etc. (conversely perhaps other passes worked around the issue in less than ideal ways and could benefit from using this new approach).

Looks like some places that could use this functionality aren't quite there yet -
The Attributor has an internalizing feature (added in 87a85f3d57f55f5652ec44f77816c7c9457545fa ) that produces invalid IR (ends up with two !dbg attachments of the same DISubprogram) if the function it's internalizing has a DISubprogram - but if it succeeded it'd have the same problem that the linkage name on the DISubprogram wouldn't match the actual symbol/function name. (@jdoerfert @bbn).
The IR Outliner (@AndrewLitteken ) seems to be only a few months old and appears to have no debug info handling - probably results in the same problem.
The Partial Inliner does clone a function into a new name & so would have an invalid DISubprogram, though it only uses the clone for inlining (this probably then goes on to produce the desired debug info, where the inlining appears to come from the original function)
ThinLTO does some function cloning within a module for aliases, but it then renames the clone to the aliasees name so I think the name works out to match again.

Another place where mismatch between linkage name and DISubprogram happens is the cloning for coroutines (.resume, .destroy and cleanup funclets). We found that out again through different kind of AutoFDO profile fidelity issue (cc: @lxfind).

If this is an invariant, that the linkage name on the DISubprogram should match the actual llvm::Function name (@aprantl @JDevlieghere - verifier check, perhaps?) - it'd be nice to make that more reliable, either by removing the name and relying on the llvm::Function name (perhaps with a boolean on the DISubprogram as to whether the linkage name should be emitted or not - I think currently Clang's IRGen makes choices about which DISubprograms will get linkage names) or a verifier and utilities to keep them in sync.

Agreed, clarity on the rules and sanity check built into verifier would be beneficial.

But I'll leave that alone for now/for this review, unless someone else wants to chime in on it.

In D93747#2469556, @hoy wrote:

In 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?