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.

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.

In D94154#2487762, @dblaikie wrote:

In D94154#2487745, @hoy wrote:

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?

Yes, same problem exists for C functions. For C functions without DW_AT_linkage_name, DW_AT_name will be used by AutoFDO and that's not uniquefied.

And it can't be added because that'd break debuggers by giving them a (correct) linkage name that they couldn't demangle.

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

In D92633#2485811, @MaskRay wrote:

In D92633#2434768, @tmsriram wrote:

In D92633#2434766, @MaskRay wrote:

In D92633#2434714, @tmsriram wrote:

Correct me if I am wrong, but I do see that this behavior is touched. Line 10 in -fdirect-access-external-data.c :

// RUN: %clang -### -c -target aarch64 %s -fpic -fdirect-access-external-data 2>&1 | FileCheck %s --check-prefix=DIRECT

With -fpic, the variable access will go directly and not via GOT.

clang/test/Driver/fdirect-access-external-data.c is a driver test which tests how the driver passes options to CC1.

clang/lib/CodeGen/CodeGenModule.cpp says how the CC1 option affects the dso_local specifier in the LLVM IR output. Currently it is a no op for -fpic/-fPIC.

Great! Sorry I didnt realize this, this is fine with me now!

(I have made some tests. It looks like implementing -fno-direct-access-external-data for -fno-pic is not an insurmountable work: ~50 tests)

May I get a formal LGTM? 😋 I am happy to give GCC devs a few more days... https://gcc.gnu.org/pipermail/gcc/2021-January/234639.html (the previous feature request and gcc/ mailing list message has given them one month and -f[no-]direct-access-external-data is still the best name so far)

In D93876#2484989, @dblaikie wrote:

In D93876#2483702, @tmsriram wrote:

In D93876#2474377, @dblaikie wrote:

This patch disables this implicit behavior. It only creates function sections for those functions that require basic block sections.

Is this necessary? I would guess even with a bb sections function, its main section could go in the generic .text section?

But we want the .text section to be unique because we want reorder functions at link time. Does that make sense?

Somewhat - but if the user wants reordering, wouldn't they want -ffunction-sections too?

I guess if they use bb-sections but not function-sections that assumes selective bb-sections would apply to all the hot functions? So that lets the hot code be reordered, and all the functions that aren't bb-split would go in one big .text section and be considered "not hot"?

In D93876#2474377, @dblaikie wrote:

This patch disables this implicit behavior. It only creates function sections for those functions that require basic block sections.

Is this necessary? I would guess even with a bb sections function, its main section could go in the generic .text section?

Address reviewer comments. Extend test to check for no function sections.

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

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.

Fix a clang test to reflect the new change.

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.

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?

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.

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.

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)

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#2470189, @hoy 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.

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

In D93082#2469154, @MaskRay wrote:

.L prefixed symbols will be discarded by the linker in the default mode (none of --discard-none/--discard-locals/--discard-all is specified).
So if you use .L (called PrivateGlobalPrefix in MC), this will work without any linker change (if possible I would hope we just make use of existing conventions instead of adding more linker rules).
For the string __uniq and __part, I am fine with them and actually prefer them if you have measured that they don't cause too mush bloat. The names are clearer than other alternatives like unary coding and special code points (e.g. LLVM IR names make use of \1)

With full basic block sections, the object size bloat from section table outweighs the bloats from strtab and symtab.

sizeof(Elf64_Shdr) = 64. A C++ mangled symbol name can be longer than it, but I don't know the average length.

Address reviewer comments.

In D93082#2469154, @MaskRay wrote:

.L prefixed symbols will be discarded by the linker in the default mode (none of --discard-none/--discard-locals/--discard-all is specified).

In D93082#2469132, @MaskRay wrote:

b) The current basic block naming just adds a ".N" to the symbol name where N is some integer. This collides with how clang creates __cxx_global_var_init.N. clang creates these symbol names to call constructor functions and basic block symbol naming should not use the same style.

This reason alone is not sufficient I guess. __part.N greatly increases .strtab usage. Are there other benefits?

Use part instead of bb. __part is more descriptive as the symbol can denote one or more basic blocks.

https://reviews.llvm.org/D73307#1932131 rnk@ mentioned this : "At a higher level, should this just be an IR pass that clang adds into the pipeline when the flag is set? It should be safe to rename internal functions and give private functions internal linkage. It would be less invasive to clang and have better separation of concerns. As written, this is based on the source filename on the module, which is accessible from IR. The only reason I can think of against this is that the debug info might refer to the function linkage name, but maybe that is calculated later during codegen."

I too was currently working on changing the raw linkage names, DW_AT_linkage_name, and was about to send out a patch along these lines :). Thanks for doing this! I will take a look at it.

In D93082#2449369, @rahmanl wrote:

It may be worthwhile to think about removing the second dot from the naming, i.e., foo.__bb.N -> foo.__bbN . The reason is that this further distinguishes the symbol name from symbols like __cxx_global_var_init.N plus it saves one character.

In D92633#2434766, @MaskRay wrote:

In D92633#2434714, @tmsriram wrote:

Correct me if I am wrong, but I do see that this behavior is touched. Line 10 in -fdirect-access-external-data.c :

// RUN: %clang -### -c -target aarch64 %s -fpic -fdirect-access-external-data 2>&1 | FileCheck %s --check-prefix=DIRECT

With -fpic, the variable access will go directly and not via GOT.

clang/test/Driver/fdirect-access-external-data.c is a driver test which tests how the driver passes options to CC1.

In D92633#2434511, @MaskRay wrote:

In D92633#2434337, @tmsriram wrote:

In D92633#2434267, @MaskRay wrote:

In D92633#2434231, @tmsriram wrote:

Sorry just one more thing which is a bit concerning:

When I do :

$ clang -fPIC -frxternal-data-access foo.c

You will omit the GOT but this object can go into a shared object and break the build as this does not apply to shared objects? Should we allow this at all for -fPIC? I dont see a need for this but if you want to, maybe warn? The user should know that this cannot go into a shared object right?
I am also ok with commenting that this option can do bad things for -fPIC and the user should know what they are doing.

clang -fPIC -fdirect-access-eternal-data -c a.c; ld -no-pie a.o; ld -pie a.o (producing an executable with either -no-pie or -pie) is fine.

Yes, agreed. Putting this object into an executable (pie/no-pie) no matter how you compile it is always fine as long as the backend supports copy relocations. No issues there.

For -shared, because an ELF linker assumes interposition for non-local STV_DEFAULT symbols by default, linking such an object files requires some mechanism to make it non-preemptible.

Right, that was my point. Without -fPIC, we can be guaranteed that it won't go into a shared object and this case wouldn't arise.

The simplest is clang -fPIC -fdirect-access-external-data -c a.c; ld -shared -Bsymbolic a.o
Other mechanisms include: --dynamic-list (don't list the symbol) and --version-script (localize the symbol).
This is going to be tricky and I don't know how to fit the information into the few-line description of the option.

How about making this option applicable for -fpie/fPIE and the default case and *not allowing* it for -fPIC/-fpic? That seems the safest approach.

I just checked how to make -fdirect-access-eternal-data work for -fno-pic (both TargetMachine::shouldAssumeDSOLocal and CodeGenModule.cpp:shouldAssumeDSOLocal should not assume false for Reloc::Static), unfortunately there are 200 tests needing updates. (This reminds me that LLVM doesn't get the default for dso_local right, so many tests have dso_local in ELF/COFF but no dso_local in Mach-O. Unfortunately it is extremely difficult to fix it today (D76561))

Ok, I lost you here a bit. This should be fine for -fno-pic was my understanding.

Let me try to summarize the motivations of this patch:

1. Originally, the default build (fno-pie/fno-pic), always used direct access for external globals resulting in copy relocations if it is truly external at link time. You want to change that to be able to not have copy relocations with -fno-direct-access-external-data, and you claim this would be useful for POWER, great!
2. Originally, with -fPIE and -fpie, -mpie-copy-relocations allowed direct access to externals. This was always safe because the object was guaranteed to go into an executable. The new option preserves this behavior so there is no concern.

Yes for 1 and 2. This patch only makes the options work for -fpie (as the original -mpie-copy-relocations does).

1 will be a nice cleanup (to drop 2 lines from TargetMachine::shouldAssumeDSOLocal) but it may require some test updates and it looks like CodeGen/MachineCSE.cpp exposes a crashing bug that it cannot handle non-dso_local external constant in -relocation-model=static mode correctly...

3. Originally, there was no way to generate direct accesses to external global variables with -fPIC/-fpic. That behavior will change if you support -fdirect-access-external-data with -fPIC. That is my concern that this adds to the complexity and the user should know what they are doing.

Given 3), I am wondering if you really need this patch. I will leave it to you but please do consider the fact that opening up this option to -fPIC might be a problem.

I agree. The behavior is not touched in this patch.

In D92633#2434267, @MaskRay wrote:

In D92633#2434231, @tmsriram wrote:

Sorry just one more thing which is a bit concerning:

When I do :

$ clang -fPIC -frxternal-data-access foo.c

You will omit the GOT but this object can go into a shared object and break the build as this does not apply to shared objects? Should we allow this at all for -fPIC? I dont see a need for this but if you want to, maybe warn? The user should know that this cannot go into a shared object right?
I am also ok with commenting that this option can do bad things for -fPIC and the user should know what they are doing.

clang -fPIC -fdirect-access-eternal-data -c a.c; ld -no-pie a.o; ld -pie a.o (producing an executable with either -no-pie or -pie) is fine.

In D92633#2434212, @MaskRay wrote:

In D92633#2434166, @tmsriram wrote:

LGTM. I checked it completely and the patch makes total sense to me.

Thanks! The name bothers me a bit. There are two parts "direct-access" (direct access relocations like absolute relocations and PC-relative relocations) "external-data" (this is for an external global data symbol with default visibility) (-fno-plt is a similar option for function symbols)

"external global data symbol with default visibility" is apparently too long and not suitable for an option name.
I do not know how to fit default into the name, so I just omit it.
I think external implies global (undefined local symbols are not allowed, either by the assembler or by the linker in various binary formats), so external-global probably does not convey more information than external-data. In the end I still think -fdirect-access-external-data is the best name I can think of...

Sorry just one more thing which is a bit concerning:

LGTM. I checked it completely and the patch makes total sense to me.

In D92633#2433979, @MaskRay wrote:

In D92633#2433108, @tmsriram wrote:

You said : "The name -mpie-copy-relocations is misleading [1] and does not capture the idea that this option can actually apply to all of -fno-pic,-fpie, ..."

Could you please clarify why this option needs to apply to -fno-pic? Here is what I tried with trunk clang:

If the user wants to guarantee no copy relocations in -fno-pic code, they can theoretically apply -fno-direct-access-external-data to use a GOT indirection.
This is not implemented, though.

extern int var;
int get() { return var; }

$ clang -S foo.c -o -
....
movl var, %eax
popq %rbp
...

With -fno-pic, this will never need to use -mpie-copy-relocations at all, so this case is not relevant right? Did I miss anything?

-fno-pic code can only be used with -no-pie links (position-dependent executables) If var is not defined in the linked executable, it will have a copy relocation.

You said : "The name -mpie-copy-relocations is misleading [1] and does not capture the idea that this option can actually apply to all of -fno-pic,-fpie, ..."

This LGTM.

Thanks for fixing this!

Abandoning it because D83655 fixes it by splitting up gcc_except_table.

In D90417#2362862, @MaskRay wrote:

I think a better way to fix this is to split up .gcc_except_table (D83655). I'll resurrect the patch.

Address the following reviewer comments:

Change the prefix to "__uniq" like mtrofin@ suggested.

In D89617#2339607, @mtrofin wrote:

In D89617#2339557, @tmsriram wrote:

Incorporating snehasishk@'s suggestion that "llvmuniq" is a better prefix for tools to search on.

Having "llvmuniq" makes reduces chances of collisions with other suffixes that might get introduced. Consistent with the ".llvm." prefix for lto global name promotion suffixes.

Unique-ing names this way isn't, at core, something that's too tied to llvm, even if we implement it here first. There are tools that would leverage this feature by binding to the suffix. With a look at other present or future compilers, and given that making this decision now is free, and much more expensive later, I think it would be beneficial if we were intentional about:

• not using "llvm" to indicate this is something tool-generated, and instead use, say "" (".uniq.")

or

• using "llvm", on the grounds that there are other examples of general-purpose concepts that have tool-specific prefixes (like gcc_except_table, that is supported in llvm); so the prefixed name is basically a proper noun, it doesn't mean "llvm only" or "gcc only".

I believe either way, the ".llvm." case is going to stick out as an inconsistency: it should have been either ".llvmlto." or "__lto", in either case, I don't think it serves too well as an argument for the llvm prefix.

Address reviewer comments, move the negative check into CodeGen.

In D19995#2340941, @MaskRay wrote:

@tmsriram I think this can be abandoned since we've achieved the optimization with dso_local (dso_local is computed from -mpie-copy-relocations and many other factors),
so we don't need to make the MO_* decision in every backend.

LGTM, thanks.

Incorporating snehasishk@'s suggestion that "llvmuniq" is a better prefix for tools to search on.

Prepend ".uniq." to the module name hash.

Remove checks of valid options in frontend as codegen already does that.

Test the error messages in the driver and the frontend.

Address reviewer comments, add UNSUPPORTED: and test one more permutation (the exact reverse) for basic block reordering.

In D89500#2333547, @dblaikie wrote:

In D89500#2333518, @tmsriram wrote:

Based on dblaikie's comment, remove the driver check for file exists completely as the front-end will do this anyways and report an appropriate error upon failure.

Is there a test that needs to be removed that was testing the driver behavior? And is there a test testing the underlying/compiler ("frontend") error behavior?

Based on dblaikie's comment, remove the driver check for file exists completely as the front-end will do this anyways and report an appropriate error upon failure.

In D89500#2333472, @dblaikie wrote:

Hmm - are other file existence checks done in the driver? (eg: for source files themselves) I'd have assumed not - would have thought we'd let the filename propagate down until it's actually opened and then failed there. (means fewer error paths - since the latter's necessary anyway, and removes the chance of racey file creation/deletion/etc producing different behavior)

Fix the diff.

Oops, let me fix the diff first.

In D89179#2329922, @labath wrote:

I'm still trying to understand what platforms is this supposed to work on -- mainly to see if additional test annotations are needed. What's the story with non-x86 non-elf targets?

In D89179#2328195, @dblaikie wrote:

In D89179#2328141, @MaskRay wrote:

I think this is like an integration test. Not all Clang supporting -fbasic-block-sections can work. This requires a Clang built at the same commit as LLDB..

Not sure I follow - why would this test need to be built with a version matched clang+lldb? I'd expect it needed a clang with at least the functionality being tested, as @labath was mentioning - but I don't /think/ it'd need a version locked clang.

Yeah, that requirement would be pretty surprising, though the way this test is implemented means that it will always run with a version locked clang. If we wanted to explicitly test different versions of the compiler (I don't think it's necessary) this would need to be written as an API test. Just note that the only existing bot configured to run this way is a mac machine, so it will not be of any immediate use anyway (I think).

Fix a typo.