LGTM, thanks.

Adding support in the priority-based inliner.

Thanks for the refactoring!

Thanks for working on this which will help the thinLTO throughput. I guess there will be a separate patch to turn on this work, i.e, setting CtxSplitLayout?

Rebasing.

Removing the UniqueDebugLinakgeNames switch.

In D93747#2488399, @dblaikie wrote:

In D93747#2488390, @tmsriram wrote:

In D93747#2488387, @dblaikie wrote:

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.

In D94154#2488088, @rnk wrote:

In D94154#2487891, @tmsriram wrote:

In D94154#2487762, @dblaikie wrote:

@rnk so I think this ^ leads us back to the "maybe this should be done in clang where we can enable mangling for C static functions when needed/when this feature is enabled". (I don't mean that we should add this scheme into the mangling scheem proper - I'm still OK with it being a ".part.N" suffix - but that that suffix should only be added to a mangled name, not to an unmangled name like a plain C function name) - not sure how that works for Windows (does Windows mangling scheme have a notion of suffixes, etc - I guess this system as implemented assumes Windows is cool with ".foo.N" suffixes?).

We are going in circles. For C functions, there is no linkage name added and it will not be newly introduced either, Hoy's patch specifically updates linkage name only when it is not null. When "overloadable" is used, linkage name is added as a mangled name for C functions too and the suffix will also make it demangleable. I don't see any issues.

I think David already approved the patch in Phabricator, feel free to land it. David isn't blocking the patch, I think he's suggesting that perhaps we should revisit the IR pass design decision in the near future.

Regarding David's suggestion to only added suffixes to mangled names, wouldn't that be problematic for plain C static functions with no mangling? Suppose you have a program with many C TUs, each of which contains a static free function foo. In order for AutoFDO to work, the compiler has to rename all those foo's to something globally unique. That will probably break some debugging functionality, but hey, the user asked for unique names, so they got them.

In D94154#2487727, @dblaikie wrote:

In D94154#2486081, @hoy wrote:

In D94154#2485197, @dblaikie wrote:

In D94154#2482425, @rnk wrote:

In D94154#2481491, @dblaikie wrote:

This is only demangler friendly if the name is already an itanium mangled name, right? (ie: the name starts with _Z) So it wouldn't work for C code?

I'm pretty sure most demangling tools such as c++filt check for a leading prefix of _+Z before demangling, so I don't think there are any concerns for C names. A reasonable demangler would pass them through as is.

Yep - but passing them through unmodified was causing problems for gdb which was demangling the mangled names as they appear in the DWARF and using the unmangled name to figure out how to do name lookup. So if the symbol wasn't getting unmangled you wouldn't be able to "break f1" instead you'd have to "break f1.__part.1" which would be an unfortunate regression in debuggability.

But it seems like that only applies if a mangled name is present in the DWARF at all - if no mangled name is present, and the debug info just gives the pretty name it works OK. Bit weird to have no record of the real symbol name in the DWARF, but so far it doesn't seem to cause any problems? So I'm OK-ish with this.

Do you have plans to fix this more generally? (I think to fix it more generally, you might need to move this feature up to clang and have clang mangle the name then add the suffix (at least for itanium mangling - not sure if windows mangling supports arbitrary suffixes like this, @rnk might know) - that way for C functions you can force/enable the mangling as is done for attribute((overloadable)) and others (I think attribute((enable_if)) also causes mangling of C functions))

That was Sriram's original idea: have the mangler do it. I reviewed the code. Modifying the mangler was fairly complicated, so I suggested doing it in a pass. The original pass also operated by appending a suffix after mangling, it just happened earlier. There isn't a good way to "embed" this uniquification into the Itanium mangling scheme, so far as I am aware.

Oh, I was still in favor of adding it after the mangling (the current "mangled.__part.number") but I thought it may be necessary to force mangling on C functions before adding the suffix, if they needed to be demangleable for debug info purposes.

That all said, I don't see any reason to block this decimilization change. It's limited in scope and an obvious improvement.

My concern was that it was layering more workarounds on a patch series that might be going in the wrong direction overall.

Anyway, sounds like, if I'm understanding/tested correctly, that the issue with unmangleability isn't about the symbol name itself but the DW_AT_linkage_name in the DWARF, so it seems like if that isn't present then there's no issue if the real symbol name can't be unmangled/back to the simple name. (but there's some reason the DW_AT_linkage_name, if present, must match the symbol name? (so it can't be the original unsuffixed mangled name))

One reason that DW_AT_linkage_name should be consistent with the real linkage name is to favor AutoFDO. DW_AT_linkage_name is used to generate profiles for inlined functions.

How's this work for C functions? Same sort of problem?

In D94154#2485197, @dblaikie wrote:

In D94154#2482425, @rnk wrote:

In D94154#2481491, @dblaikie wrote:

This is only demangler friendly if the name is already an itanium mangled name, right? (ie: the name starts with _Z) So it wouldn't work for C code?

I'm pretty sure most demangling tools such as c++filt check for a leading prefix of _+Z before demangling, so I don't think there are any concerns for C names. A reasonable demangler would pass them through as is.

Yep - but passing them through unmodified was causing problems for gdb which was demangling the mangled names as they appear in the DWARF and using the unmangled name to figure out how to do name lookup. So if the symbol wasn't getting unmangled you wouldn't be able to "break f1" instead you'd have to "break f1.__part.1" which would be an unfortunate regression in debuggability.

But it seems like that only applies if a mangled name is present in the DWARF at all - if no mangled name is present, and the debug info just gives the pretty name it works OK. Bit weird to have no record of the real symbol name in the DWARF, but so far it doesn't seem to cause any problems? So I'm OK-ish with this.

Do you have plans to fix this more generally? (I think to fix it more generally, you might need to move this feature up to clang and have clang mangle the name then add the suffix (at least for itanium mangling - not sure if windows mangling supports arbitrary suffixes like this, @rnk might know) - that way for C functions you can force/enable the mangling as is done for attribute((overloadable)) and others (I think attribute((enable_if)) also causes mangling of C functions))

That was Sriram's original idea: have the mangler do it. I reviewed the code. Modifying the mangler was fairly complicated, so I suggested doing it in a pass. The original pass also operated by appending a suffix after mangling, it just happened earlier. There isn't a good way to "embed" this uniquification into the Itanium mangling scheme, so far as I am aware.

Oh, I was still in favor of adding it after the mangling (the current "mangled.__part.number") but I thought it may be necessary to force mangling on C functions before adding the suffix, if they needed to be demangleable for debug info purposes.

That all said, I don't see any reason to block this decimilization change. It's limited in scope and an obvious improvement.

My concern was that it was layering more workarounds on a patch series that might be going in the wrong direction overall.

Anyway, sounds like, if I'm understanding/tested correctly, that the issue with unmangleability isn't about the symbol name itself but the DW_AT_linkage_name in the DWARF, so it seems like if that isn't present then there's no issue if the real symbol name can't be unmangled/back to the simple name. (but there's some reason the DW_AT_linkage_name, if present, must match the symbol name? (so it can't be the original unsuffixed mangled name))

Thanks for the quick fix. LGTM.

In D94154#2483336, @tmsriram wrote:

https://github.com/gcc-mirror/gcc/blob/master/libiberty/cp-demangle.c#L3863

The demangler code for suffixes that accepts either lowercase letters with underscore or numbers.

In D93747#2482730, @tmsriram wrote:

In D93747#2482726, @hoy wrote:

In D93747#2482519, @tmsriram wrote:

In D93747#2481494, @dblaikie wrote:

In D93747#2481383, @hoy wrote:

In D93747#2481304, @tmsriram wrote:

In D93747#2481223, @tmsriram wrote:

In D93747#2481163, @dblaikie wrote:

In D93747#2481095, @hoy wrote:

In D93747#2481073, @dblaikie 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?

Renaming debug linkage name if it preexisits.

In D93747#2482519, @tmsriram wrote:

In D93747#2481494, @dblaikie wrote:

In D93747#2481383, @hoy wrote:

In D93747#2481304, @tmsriram wrote:

In D93747#2481223, @tmsriram wrote:

In D93747#2481163, @dblaikie wrote:

In D93747#2481095, @hoy wrote:

In D93747#2481073, @dblaikie 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.

Thanks for the quick fix!

In D93747#2481304, @tmsriram wrote:

In D93747#2481223, @tmsriram wrote:

In D93747#2481163, @dblaikie wrote:

In D93747#2481095, @hoy wrote:

In D93747#2481073, @dblaikie wrote:

In D93747#2481053, @tmsriram wrote:

In D93747#2480442, @dblaikie wrote:

@tmsriram - any ideas what your case/example was like that might've caused degraded debugging experience? Would be good to understand if we're producing some bad DWARF with this patch/if there might be some way to avoid it (as it seems like gdb can handle mangled names/overloading in C in this example I've tried)

I haven't seen anything that caused degraded debugging experience. I am interested in this as we do look at this field for the purposes of profile attribtution for calls that are inlined. My comment was that we don't need to create this if it didn't already exist. I am not fully aware of what would happen if we did it all the time.

Ah, sorry, I got confused as to who's comment I was reading. I see it was @hoy who said:

If set, the gdb debugger will use that field to match the user input and set breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name prevents the debugger from setting a breakpoint based on source names unless the user specifies a decorated name.

Hence, this approach sounds like a workaround for us when the profile quality matters more than debugging experience.

So I'm still a bit confused. My test doesn't seem to demonstrate the issue with setting a linkage name preventing the debugger from setting a breakpoint based on the source name?

Suggesting that gdb isn't using the DW_AT_name at all for "break <function name>" but instead demangling and stripping off the extras from the linkage name, and since it can't demangle this uniquified name (unlike the mangled name used when using the overloadable attribute) that degrades the debugger user experience? I'd have to test that in more detail/with some hand-hacked DWARF.

Yes, I think in your case the linage name can be demangled by the debugger. In my previous experiment, the uniquefied names could not be demangled therefore I was not able to breakpoint.

Ah, did some more testing. Seems it's because it isn't a classically mangled name.

The simplest fix I can think of is to emit the base 10 number of the md5hash. That would preserve all the existing uniqueness and be demangler friendly. @hoy @dblaikie WDYT?

In D93747#2481073, @dblaikie wrote:

In D93747#2481053, @tmsriram wrote:

In D93747#2480442, @dblaikie wrote:

@tmsriram - any ideas what your case/example was like that might've caused degraded debugging experience? Would be good to understand if we're producing some bad DWARF with this patch/if there might be some way to avoid it (as it seems like gdb can handle mangled names/overloading in C in this example I've tried)

I haven't seen anything that caused degraded debugging experience. I am interested in this as we do look at this field for the purposes of profile attribtution for calls that are inlined. My comment was that we don't need to create this if it didn't already exist. I am not fully aware of what would happen if we did it all the time.

Ah, sorry, I got confused as to who's comment I was reading. I see it was @hoy who said:

If set, the gdb debugger will use that field to match the user input and set breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name prevents the debugger from setting a breakpoint based on source names unless the user specifies a decorated name.

Hence, this approach sounds like a workaround for us when the profile quality matters more than debugging experience.

So I'm still a bit confused. My test doesn't seem to demonstrate the issue with setting a linkage name preventing the debugger from setting a breakpoint based on the source name?

Suggesting that gdb isn't using the DW_AT_name at all for "break <function name>" but instead demangling and stripping off the extras from the linkage name, and since it can't demangle this uniquified name (unlike the mangled name used when using the overloadable attribute) that degrades the debugger user experience? I'd have to test that in more detail/with some hand-hacked DWARF.

In D93747#2481053, @tmsriram wrote:

In D93747#2480442, @dblaikie wrote:

In D93747#2470178, @tmsriram 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.

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.

In D94019#2478284, @dblaikie wrote:

In D94019#2478277, @hoy wrote:

In D94019#2478253, @dblaikie wrote:

In D94019#2478206, @hoy wrote:

In D94019#2478048, @dblaikie wrote:

In D94019#2478047, @hoy wrote:

In D94019#2478045, @dblaikie wrote:

Please add a clang test for this.

There is the original clang test unique-internal-linkage-names.cpp that still works with the change here. What kind of new test would you like?

Something that tests this change (something that would fail before this patch is applied, and passes afterwards - demonstrating the change in behavior). Either something like the LLVM test, testing the pass sequence, or something with very simple IR (something that can robustly demonstrate the change in optimization behavior due to this patch and will be resilient to as many other changes to LLVM as possible (ie: something that captures the essence of this change)).

I'm not sure how to test this change if LLVM is treated as a black box to clang. This patch doesn't seem to change anything fundamental from the Clang point of view. My understanding is that the previous placement of the unique name pass in the pipeline didn't seem particularly important to Clang, therefore we ended up just testing the whole Clang output. If that's the case, I don't think we need to test the pass order here with this change.

My understanding was that the change in pass ordering has some observable change in the total behavior of LLVM? So an end-to-end test could be used here (I tend towards preferring the checking the pass ordering, but understand @aeubanks contrasting position) - that's what I was trying to describe in the "or something with very simple IR (... )" part of my suggestion. If the change in pass order can be observed in terms of different final IR, that's what an end-to-end Clang test could validate.

The change on the LLVM side is observable on the IR level when pseudo probe is enabled. I think that LLVM change is transparent to Clang. In other words, Clang doesn't need to know that the LLVM change is to favor pseudo probe.

Clang's overall, user-facing behavior changed (otherwise why make this change, right?) due to a change to Clang's source code, so it should be tested to demonstrate that the source change had the desired effect and doesn't regress. These particular changes are awkward because things like CodeGenOpts aren't serialized, unlike LLVM IR, but that doesn't mean they shouldn't be tested - it means the testing isn't as elegant as it is for serialized IR.

In D94019#2478253, @dblaikie wrote:

In D94019#2478206, @hoy wrote:

In D94019#2478048, @dblaikie wrote:

In D94019#2478047, @hoy wrote:

In D94019#2478045, @dblaikie wrote:

Please add a clang test for this.

There is the original clang test unique-internal-linkage-names.cpp that still works with the change here. What kind of new test would you like?

Something that tests this change (something that would fail before this patch is applied, and passes afterwards - demonstrating the change in behavior). Either something like the LLVM test, testing the pass sequence, or something with very simple IR (something that can robustly demonstrate the change in optimization behavior due to this patch and will be resilient to as many other changes to LLVM as possible (ie: something that captures the essence of this change)).

I'm not sure how to test this change if LLVM is treated as a black box to clang. This patch doesn't seem to change anything fundamental from the Clang point of view. My understanding is that the previous placement of the unique name pass in the pipeline didn't seem particularly important to Clang, therefore we ended up just testing the whole Clang output. If that's the case, I don't think we need to test the pass order here with this change.

My understanding was that the change in pass ordering has some observable change in the total behavior of LLVM? So an end-to-end test could be used here (I tend towards preferring the checking the pass ordering, but understand @aeubanks contrasting position) - that's what I was trying to describe in the "or something with very simple IR (... )" part of my suggestion. If the change in pass order can be observed in terms of different final IR, that's what an end-to-end Clang test could validate.

In D94019#2478048, @dblaikie wrote:

In D94019#2478047, @hoy wrote:

In D94019#2478045, @dblaikie wrote:

Please add a clang test for this.

There is the original clang test unique-internal-linkage-names.cpp that still works with the change here. What kind of new test would you like?

Something that tests this change (something that would fail before this patch is applied, and passes afterwards - demonstrating the change in behavior). Either something like the LLVM test, testing the pass sequence, or something with very simple IR (something that can robustly demonstrate the change in optimization behavior due to this patch and will be resilient to as many other changes to LLVM as possible (ie: something that captures the essence of this change)).

In D94019#2478045, @dblaikie wrote:

Please add a clang test for this.

Replacing linkage names for debug declaration as well.

In D93747#2475852, @dblaikie wrote:

In D93747#2470504, @hoy wrote:

In D93747#2470387, @dblaikie 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#2470178, @tmsriram 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.

Was this thread concluded? Doesn't look like any check was added?

Adding a switch.

Addressing comments from dblaikie.

In D93656#2475856, @dblaikie wrote:

Looks good - test cases might benefit from some descriptive comments (explaining why the pseudo probe pass needs to be enabled to test the unique linkage name pass - I guess to check that it appears in just the right place in the pass pipeline?)

Adding a test for the new pseudo probe test.

Removing the checks of VerifierAnalysis in test code.

Removing unnecessary test code.

Adding PTO checks in LLVM test.

Undoing changes to the clang test.

In D93747#2470387, @dblaikie 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)

In D93747#2470178, @tmsriram 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.

Checking pipeline in clang test.

In D93656#2469504, @dblaikie wrote:

In D93656#2469485, @hoy wrote:

In D93656#2468841, @aeubanks wrote:

In D93656#2468834, @hoy wrote:

In D93656#2468821, @aeubanks wrote:

Also it looks like this is doing 2 different things, the moving of things from Clang to LLVM's PassBuilder, and separately the change to the pass itself. Can these be separated?

I'm not sure about a good way to separate them. There are Clang tests that may fail with removing the pass from clang while not adding it correspondingly in llvm. Adding the pass in llvm while not removing it from Clang may cause the pass to run twice which may also fail the Clang tests. What do you think?

I mean keep that in one change, but separate out the change to llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp and DebugInfoMetadata.h.

I see, thanks for the clarification.

In D93656#2468698, @dblaikie wrote:

This should have a LLVM test coverage for the LLVM changes. (I realize they're also tested by the Clang test, because there's no way to test Clang's pass manager creation short of testing the effect of running the pass manager (hmm - /maybe/ there's a way to dump the pass pipeline? In which case that's how Clang should be tested, just testing that it creates the right pipeline, not that that pipeline does any particular thing))

Added an IR test. There is the llvm switch -debug-pass= that can dump the pass pipeline. I'm not aware of a clang switch that can do that.

Seems some clang tests use something like that, eg:

clang/test/CodeGen/thinlto-debug-pm.c:// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-obj -O0 -o %t2.o -x ir %t.o -fthinlto-index=%t.thinlto.bc -fno-experimental-new-pass-manager -mllvm -debug-pass=Structure 2>&1 | FileCheck %s --check-prefix=O0-OLDPM

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

The changes that rename debug linkage name has been separated as D93747. I'm moving the discussion there.

Fixing IR test failure.

Separated PassBuilder changes.

In D93656#2468841, @aeubanks wrote:

In D93656#2468834, @hoy wrote:

In D93656#2468821, @aeubanks wrote:

Also it looks like this is doing 2 different things, the moving of things from Clang to LLVM's PassBuilder, and separately the change to the pass itself. Can these be separated?

I'm not sure about a good way to separate them. There are Clang tests that may fail with removing the pass from clang while not adding it correspondingly in llvm. Adding the pass in llvm while not removing it from Clang may cause the pass to run twice which may also fail the Clang tests. What do you think?

I mean keep that in one change, but separate out the change to llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp and DebugInfoMetadata.h.

In D93656#2468821, @aeubanks wrote:

Also it looks like this is doing 2 different things, the moving of things from Clang to LLVM's PassBuilder, and separately the change to the pass itself. Can these be separated?

Adding LTO, linker and clang supports.

Addressing feedbacks.