In D85782#2273938, @hans wrote:

In D85782#2273402, @bd1976llvm wrote:

@hans, I think that we should put this change onto the release branch for llvm11.

I'm not familiar with the details of this patch, can you explain why we should merge it to llvm11?

We're very late in the release process, so unless it's fixing some major problem, I would be hesitant to merge.

@hans, I think that we should put this change onto the release branch for llvm11.

ping!

In D85782#2236493, @MaskRay wrote:

tl;dr This is a longstanding GNU ld bug introduced in binutils 2.26

% cat a.c
__attribute__((visibility("protected"))) void * foo (void) { return (void *)foo; }
% gcc -fpic a.c -shared -fuse-ld=bfd # relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object

clang does not behave worse than GCC+GNU ld, so there is no regression on our side.

In D85782#2235567, @skan wrote:

I think this introduces an old issue when lowering the instruction.

ping!

In D85782#2211672, @bd1976llvm wrote:

In D85782#2211636, @MaskRay wrote:

GNU now recommends -fvisiblity=hidden (https://gcc.gnu.org/wiki/Visibility) so my understanding is that there will be very few STV_DEFAULT symbols.

I am not very sure this is now recommended but is indeed a non-rare configuration we should care about.

Agreed. Recommended is probably too strong.

Moved testing into linux-preemption.ll

In D85782#2211636, @MaskRay wrote:

The additional test file places too much burden on people maintaining the tests in the future. I think an additional @hidden_data = hidden global in CodeGen/X86/linux-preemption.ll should be sufficient.

In D73230#2208470, @MaskRay wrote:

The proposed canBenefitFromLocalAlias change looks good to me. Mind sending a patch?

In D73230#2208470, @MaskRay wrote:

The proposed canBenefitFromLocalAlias change looks good to me. Mind sending a patch?

In D73230#2207837, @MaskRay wrote:

In D73230#2207735, @bd1976llvm wrote:

In D73230#2207634, @MaskRay wrote:

In D73230#2207477, @bd1976llvm wrote:

So, clang's behavior has changed so that --wrap no longer wraps symbol definitions for default symbols + -fpic + -fno-semantic-interposition (-fno-semantic-interposition is the default); however, you can restore the old behavior via -fsemantic-interposition. For hidden symbols + -fpic --wrap no longer wraps symbol definitions and there is no way to restore the old definition wrapping behavior.

I think the summary is correct. -fvisibility=hidden nullifies -fsemantic-interposition when the definition is available in the same translation unit. Given how GCC and GNU ld handle/document it, I'd say the previous hidden clang behavior working with -Wl,--wrap=foo is accidental rather than intentional. If you don't pass explicit -fsemantic-interposition, in -fPIC mode clang can freely inline foo into call sites, which will also defeat the intended -Wl,--wrap=foo behavior.

Any undefined reference to symbol will be resolved to "__wrap_symbol".

I think the reasonably portable approach making the wrapping scheme work is __attribute__((weak)). An alternative is to move the definitions to a separate translation unit (it does not work with -r or GCC LTO, though).

.. but now we have this difference in behaviour for -normal vs flto links:

ben@ben-VirtualBox:~/tests/wrap$ cat smaller.c 
void __wrap_foo () {
	puts ("__wrap_foo");
	__real_foo();
}

void foo () { puts("foo()"); }

int main() { foo(); }
ben@ben-VirtualBox:~/tests/wrap$ clang smaller.c -Wno-implicit-function-declaration -fpic -ffunction-sections -Wl,--wrap=foo -o old.elf -fvisibility=hidden -fuse-ld=lld
ben@ben-VirtualBox:~/tests/wrap$ ./old.elf
__wrap_foo
foo()
ben@ben-VirtualBox:~/tests/wrap$ clang smaller.c -Wno-implicit-function-declaration -fpic -ffunction-sections -Wl,--wrap=foo -o old_lto.elf -fvisibility=hidden -fuse-ld=lld -flto
ben@ben-VirtualBox:~/tests/wrap$ ./old_lto.elf
__wrap_foo
foo()
ben@ben-VirtualBox:~/tests/wrap$ ~/u/build/bin/clang smaller.c -Wno-implicit-function-declaration -fpic -ffunction-sections -Wl,--wrap=foo -o new.elf -fvisibility=hidden -fuse-ld=lld
ben@ben-VirtualBox:~/tests/wrap$ ./new.elf
foo()
ben@ben-VirtualBox:~/tests/wrap$ ~/u/build/bin/clang smaller.c -Wno-implicit-function-declaration -fpic -ffunction-sections -Wl,--wrap=foo -o new_lto.elf -fvisibility=hidden -fuse-ld=lld -flto
ben@ben-VirtualBox:~/tests/wrap$ ./new_lto.elf
__wrap_foo
foo()

... and the same will be true for default symbols + -fpic when we enable -fno-semantic-interposition by default for -fpic :(

I think the non-LTO vs Full LTO difference is due to the way we implement --wrap for LTO: D33621.
It adds WeakAny linkage which obtains wrapping behavior.

clang -fuse-ld=lld smaller.c -Wno-implicit-function-declaration -fpic -ffunction-sections -Wl,--wrap=foo -fvisibility=hidden -flto -Wl,--save-temps

% llvm-dis < a.out.0.0.preopt.bc
...
define weak hidden void @foo() #0 {  # Note the weakany linkage
...

I think the only way making --wrap sufficiently reliable is to communicate --wrap to the LTO code generation. It was considered unworthy at the implementation time https://bugs.llvm.org/show_bug.cgi?id=33145#c7

To make the non-LTO case behave like the LTO case, add __attribute__((weak))

In D73230#2207634, @MaskRay wrote:

In D73230#2207477, @bd1976llvm wrote:

So, clang's behavior has changed so that --wrap no longer wraps symbol definitions for default symbols + -fpic + -fno-semantic-interposition (-fno-semantic-interposition is the default); however, you can restore the old behavior via -fsemantic-interposition. For hidden symbols + -fpic --wrap no longer wraps symbol definitions and there is no way to restore the old definition wrapping behavior.

I think the summary is correct. -fvisibility=hidden nullifies -fsemantic-interposition when the definition is available in the same translation unit. Given how GCC and GNU ld handle/document it, I'd say the previous hidden clang behavior working with -Wl,--wrap=foo is accidental rather than intentional. If you don't pass explicit -fsemantic-interposition, in -fPIC mode clang can freely inline foo into call sites, which will also defeat the intended -Wl,--wrap=foo behavior.

Any undefined reference to symbol will be resolved to "__wrap_symbol".

I think the reasonably portable approach making the wrapping scheme work is __attribute__((weak)). An alternative is to move the definitions to a separate translation unit (it does not work with -r or GCC LTO, though).

In D73230#2207477, @bd1976llvm wrote:

In D73230#2207258, @MaskRay wrote:

In D73230#2207159, @bd1976llvm wrote:

In D73230#2207023, @MaskRay wrote:

In D73230#2206232, @bd1976llvm wrote:

@MaskRay - this change causes a behaviour difference for --wrap.

Here is the --wrap behaviour before this change:

ben@ben-VirtualBox:~/tests/wrap$ more main.c
void __wrap_foo () {
	puts ("__wrap_foo");
	__real_foo();
}

void foo () { puts("foo()"); }

int main() {
	__real_foo();
	puts("---");
	__wrap_foo();
	puts("---");
	foo();
	return 0;
}
ben@ben-VirtualBox:~/tests/wrap$ gcc main.c -Wl,--wrap=foo -ffunction-sections -fuse-ld=lld -o lld.elf -Wno-implicit-function-declaration
ben@ben-VirtualBox:~/tests/wrap$ ./lld.elf 
foo()
---
__wrap_foo
foo()
---
__wrap_foo
foo()

… and here is the behaviour after this change:

ben@ben-VirtualBox:~/tests/wrap$ ./lld.elf 
foo()
---
__wrap_foo
foo()
---
foo()

There is no behaviour change for -flto builds so the behaviour for --wrap is now effectively different for LTO vs normal builds.

I think you missed a point in the description of --wrap:

You may wish to provide a "__real_malloc" function as well, so that links without the --wrap option will succeed.  If you do this, you
should not put the definition of "__real_malloc" in the same file as "__wrap_malloc"; if you do, the assembler may resolve the call
before the linker has a chance to wrap it to "malloc".

Providing foo definition in the translation unit where they are referenced is not reliable when you are using --wrap.
Actually, this is where GNU ld and LLD differ. See https://sourceware.org/bugzilla/show_bug.cgi?id=26358 and the history of lld/test/ELF/wrap-shlib-undefined.s

If you want to get guaranteed semantics, don't define foo when it is referenced. You may also try gcc and gcc -fPIC -fno-semantic-interposition, the behavior is similar to latest clang.

Thanks for the summary. I am not particularly concerned about which behaviour we have w.r.t. wrapping intra-translation-unit references (although I have seen some evidence that lld's behaviour is useful e.g. https://stackoverflow.com/questions/13961774/gnu-gcc-ld-wrapping-a-call-to-symbol-with-caller-and-callee-defined-in-the-sam). However, you stated in https://sourceware.org/bugzilla/show_bug.cgi?id=26358 that for lld -r, lto, and normal links have the same behaviour - that is not true after this change. Furthermore, with the current clang it is not possible to go back to the old behaviour using -fsemantic-interposition for hidden symbols. IIUC I think that hidden symbols are probably the majority of opensource symbols now as the GNU toolchain encourages the use of -fvisiblity=hidden.

Let me summarize the cases:

• {clang,gcc} -fuse-ld=lld main.c -Wl,--wrap=foo => wrapped (in LLD, --wrap is done after (global) symbol resolution. Definitions are wrapped as well)
• {clang,gcc} -fuse-ld=bfd main.c -Wl,--wrap=foo => not wrapped (in GNU ld, --wrap is per object file. --wrap is not effective when the symbol is defined)
• {clang,gcc} -fuse-ld=lld main.c -fPIC -fno-semantic-interposition -Wl,--wrap=foo => not wrapped (references go through .Lfoo$local which cannot be wrapped)

I think your make an integration between this commit and 872c5fb1432493c0a09b6f210765c0d94ce9b5d0, so for -fno-PIC or -fPIE, you observe the -fPIC -fno-semantic-interposition behavior as well. If you cherry pick 872c5fb1432493c0a09b6f210765c0d94ce9b5d0 and don't use -fno-semantic-interposition, and use LLD, you should get a wrapping behavior. (Clang traditionally has some -fno-semantic-interposition behaviors, so in the future we might be able to make -fno-semantic-interposition default for -fPIC.)

I indeed prefer the LLD behavior, so I filed https://sourceware.org/bugzilla/show_bug.cgi?id=26358 yesterday, but I cannot say the wrapping behavior is promised. If you want better portability, make foo weak (and be aware of side effects with the change).

@MaskRay - Thanks for taking the time to look into this :)

Here are my results from an e912fffd3a8c6c9f6e09d2eac4c1ee3a32800a22 clang using my previous example. I have included a bit more context in the first relocation dump so you can see which relocation.. in the other dumps I strip out more.

With -fpic:

$ /c/u/br2/bin/clang main.c -fno-semantic-interposition -c -fpic -target x86_64-linux-gnu -o test.o -ffunction-sections -fdata-sections -fvisibility=default -Wno-implicit-function-declaration
$ /c/u/br2/bin/llvm-readelf -r test.o                                           
...
Relocation section '.rela.text.main' at offset 0x330 contains 7 entries:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
...
0000000000000041  0000000600000004 R_X86_64_PLT32         0000000000000000 .text.foo - 4
...
Relocation section '.rela.eh_frame' at offset 0x3d8 contains 3 entries:
...

$ /c/u/br2/bin/clang main.c -fsemantic-interposition -c -fpic -target x86_64-linux-gnu -o test.o -ffunction-sections -fdata-sections -fvisibility=default -Wno-implicit-function-declaration
$ /c/u/br2/bin/llvm-readelf -r test.o                                           
...
0000000000000041  0000000a00000004 R_X86_64_PLT32         0000000000000000 foo - 4
...

$ /c/u/br2/bin/clang main.c -fno-semantic-interposition -c -fpic -target x86_64-linux-gnu -o test.o -ffunction-sections -fdata-sections -fvisibility=hidden -Wno-implicit-function-declaration
$ /c/u/br2/bin/llvm-readelf -r test.o                                           
...
0000000000000041  0000000600000004 R_X86_64_PLT32         0000000000000000 .text.foo - 4
...

$ /c/u/br2/bin/clang main.c -fsemantic-interposition -c -fpic -target x86_64-linux-gnu -o test.o -ffunction-sections -fdata-sections -fvisibility=hidden -Wno-implicit-function-declaration
$ /c/u/br2/bin/llvm-readelf -r test.o                                           
...
0000000000000041  0000000600000004 R_X86_64_PLT32         0000000000000000 .text.foo - 4
…

With -fpie:

$ /c/u/br2/bin/clang main.c -fno-semantic-interposition -c -fpie -target x86_64-linux-gnu -o test.o -ffunction-sections -fdata-sections -fvisibility=default -Wno-implicit-function-declaration
$ /c/u/br2/bin/llvm-readelf -r test.o
...
0000000000000041  0000000a00000004 R_X86_64_PLT32         0000000000000000 foo - 4
...

$ /c/u/br2/bin/clang main.c -fsemantic-interposition -c -fpie -target x86_64-linux-gnu -o test.o -ffunction-sections -fdata-sections -fvisibility=default -Wno-implicit-function-declaration
$ /c/u/br2/bin/llvm-readelf -r test.o                                           
...
0000000000000041  0000000a00000004 R_X86_64_PLT32         0000000000000000 foo - 4
...

$ /c/u/br2/bin/clang main.c -fno-semantic-interposition -c -fpie -target x86_64-linux-gnu -o test.o -ffunction-sections -fdata-sections -fvisibility=hidden -Wno-implicit-function-declaration
$ /c/u/br2/bin/llvm-readelf -r test.o                                           
...
0000000000000041  0000000a00000004 R_X86_64_PLT32         0000000000000000 foo - 4
...

$ /c/u/br2/bin/clang main.c -fsemantic-interposition -c -fpie -target x86_64-linux-gnu -o test.o -ffunction-sections -fdata-sections -fvisibility=hidden -Wno-implicit-function-declaration
$ /c/u/br2/bin/llvm-readelf -r test.o                                           
...
0000000000000041  0000000a00000004 R_X86_64_PLT32         0000000000000000 foo - 4
...

So, clang's behavior has changed so that --wrap no longer wraps symbol definitions for default symbols + -fpic + -fno-semantic-interposition (-fno-semantic-interposition is the default); however, you can restore the old behavior via -fsemantic-interposition. For hidden symbols + -fpic --wrap no longer wraps symbol definitions and there is no way to restore the old definition wrapping behavior.

In D73230#2207258, @MaskRay wrote:

In D73230#2207159, @bd1976llvm wrote:

In D73230#2207023, @MaskRay wrote:

In D73230#2206232, @bd1976llvm wrote:

@MaskRay - this change causes a behaviour difference for --wrap.

Here is the --wrap behaviour before this change:

ben@ben-VirtualBox:~/tests/wrap$ more main.c
void __wrap_foo () {
	puts ("__wrap_foo");
	__real_foo();
}

void foo () { puts("foo()"); }

int main() {
	__real_foo();
	puts("---");
	__wrap_foo();
	puts("---");
	foo();
	return 0;
}
ben@ben-VirtualBox:~/tests/wrap$ gcc main.c -Wl,--wrap=foo -ffunction-sections -fuse-ld=lld -o lld.elf -Wno-implicit-function-declaration
ben@ben-VirtualBox:~/tests/wrap$ ./lld.elf 
foo()
---
__wrap_foo
foo()
---
__wrap_foo
foo()

… and here is the behaviour after this change:

ben@ben-VirtualBox:~/tests/wrap$ ./lld.elf 
foo()
---
__wrap_foo
foo()
---
foo()

There is no behaviour change for -flto builds so the behaviour for --wrap is now effectively different for LTO vs normal builds.

I think you missed a point in the description of --wrap:

You may wish to provide a "__real_malloc" function as well, so that links without the --wrap option will succeed.  If you do this, you
should not put the definition of "__real_malloc" in the same file as "__wrap_malloc"; if you do, the assembler may resolve the call
before the linker has a chance to wrap it to "malloc".

Providing foo definition in the translation unit where they are referenced is not reliable when you are using --wrap.
Actually, this is where GNU ld and LLD differ. See https://sourceware.org/bugzilla/show_bug.cgi?id=26358 and the history of lld/test/ELF/wrap-shlib-undefined.s

If you want to get guaranteed semantics, don't define foo when it is referenced. You may also try gcc and gcc -fPIC -fno-semantic-interposition, the behavior is similar to latest clang.

Thanks for the summary. I am not particularly concerned about which behaviour we have w.r.t. wrapping intra-translation-unit references (although I have seen some evidence that lld's behaviour is useful e.g. https://stackoverflow.com/questions/13961774/gnu-gcc-ld-wrapping-a-call-to-symbol-with-caller-and-callee-defined-in-the-sam). However, you stated in https://sourceware.org/bugzilla/show_bug.cgi?id=26358 that for lld -r, lto, and normal links have the same behaviour - that is not true after this change. Furthermore, with the current clang it is not possible to go back to the old behaviour using -fsemantic-interposition for hidden symbols. IIUC I think that hidden symbols are probably the majority of opensource symbols now as the GNU toolchain encourages the use of -fvisiblity=hidden.

Let me summarize the cases:

• {clang,gcc} -fuse-ld=lld main.c -Wl,--wrap=foo => wrapped (in LLD, --wrap is done after (global) symbol resolution. Definitions are wrapped as well)
• {clang,gcc} -fuse-ld=bfd main.c -Wl,--wrap=foo => not wrapped (in GNU ld, --wrap is per object file. --wrap is not effective when the symbol is defined)
• {clang,gcc} -fuse-ld=lld main.c -fPIC -fno-semantic-interposition -Wl,--wrap=foo => not wrapped (references go through .Lfoo$local which cannot be wrapped)

I think your make an integration between this commit and 872c5fb1432493c0a09b6f210765c0d94ce9b5d0, so for -fno-PIC or -fPIE, you observe the -fPIC -fno-semantic-interposition behavior as well. If you cherry pick 872c5fb1432493c0a09b6f210765c0d94ce9b5d0 and don't use -fno-semantic-interposition, and use LLD, you should get a wrapping behavior. (Clang traditionally has some -fno-semantic-interposition behaviors, so in the future we might be able to make -fno-semantic-interposition default for -fPIC.)

I indeed prefer the LLD behavior, so I filed https://sourceware.org/bugzilla/show_bug.cgi?id=26358 yesterday, but I cannot say the wrapping behavior is promised. If you want better portability, make foo weak (and be aware of side effects with the change).

In D73230#2207023, @MaskRay wrote:

In D73230#2206232, @bd1976llvm wrote:

@MaskRay - this change causes a behaviour difference for --wrap.

Here is the --wrap behaviour before this change:

ben@ben-VirtualBox:~/tests/wrap$ more main.c
void __wrap_foo () {
	puts ("__wrap_foo");
	__real_foo();
}

void foo () { puts("foo()"); }

int main() {
	__real_foo();
	puts("---");
	__wrap_foo();
	puts("---");
	foo();
	return 0;
}
ben@ben-VirtualBox:~/tests/wrap$ gcc main.c -Wl,--wrap=foo -ffunction-sections -fuse-ld=lld -o lld.elf -Wno-implicit-function-declaration
ben@ben-VirtualBox:~/tests/wrap$ ./lld.elf 
foo()
---
__wrap_foo
foo()
---
__wrap_foo
foo()

… and here is the behaviour after this change:

ben@ben-VirtualBox:~/tests/wrap$ ./lld.elf 
foo()
---
__wrap_foo
foo()
---
foo()

There is no behaviour change for -flto builds so the behaviour for --wrap is now effectively different for LTO vs normal builds.

I think you missed a point in the description of --wrap:

You may wish to provide a "__real_malloc" function as well, so that links without the --wrap option will succeed.  If you do this, you
should not put the definition of "__real_malloc" in the same file as "__wrap_malloc"; if you do, the assembler may resolve the call
before the linker has a chance to wrap it to "malloc".

Providing foo definition in the translation unit where they are referenced is not reliable when you are using --wrap.
Actually, this is where GNU ld and LLD differ. See https://sourceware.org/bugzilla/show_bug.cgi?id=26358 and the history of lld/test/ELF/wrap-shlib-undefined.s

If you want to get guaranteed semantics, don't define foo when it is referenced. You may also try gcc and gcc -fPIC -fno-semantic-interposition, the behavior is similar to latest clang.

@MaskRay - this change causes a behaviour difference for --wrap.

Addressed review comments.

BTW - I realised that this function needs formatting correctly :(

Share logic between the two code paths.

In D83069#2129923, @jhenderson wrote:

You said to me offline when you were discussing this that you were concerned by performance implications. I take it this isn't the approach you had then, or you changed your mind?

Thanks for the suggested improvements.

Add back python improvement that was somehow dropped during test rename!

Thanks James - you were correct that the test was not being executed!

Addressed review comments.

Thanks to everyone for reviewing/guiding me though this change. When I started this I hadn't looked at lowering in LLVM in any detail so thanks for sticking with reviewing the code when I was basically learning on the job.

I would like to keep this as part of this patch.

Thanks for the interdiff rationale. Please take another look!

• Removed Clang parts of the diff. Error behaviour from llc and clang is still acceptable without the Clang parts. A generic "error: <reason>" is issued rather than a "backend" or "lowering" error as we had before... but the message contents should still allow for easy diagnosis. I will put up the Clang parts as a separate diff.
• Rebased the patch.

Use IR rather than C for the clang test.
I was able to simplify the clang patch somewhat as a "lowering error" isn't any more revealing than just emitting a generic "backend error".

In D72194#1965731, @MaskRay wrote:

I am mostly fine with the LLVM CodeGen/MC side change, but the clang side change can probably be moved to a subsequent change. For the test/clang/CodeGen/ test, we may want to test .ll -> .s instead, not .c -> .s

Report error using LLVM diagnostics framework.

@nickdesaulniers and @MaskRay,

use reportError instead of report_fatal_error.
Split testing of error and output to defeat redirection race.

In rG8cedf0e2994c1a258902ed931abdec5f94725a55#891201, @arsenm wrote:

In rG8cedf0e2994c1a258902ed931abdec5f94725a55#891041, @ychen wrote:

In rG8cedf0e2994c1a258902ed931abdec5f94725a55#890965, @arsenm wrote:

Thanks for the comments, @arsenm. I expected this would cause concerns for practical reasons. I do think it is heading the right direction though.

I have a problem with this change. IMO the entire reason to use report_fatal_error is to not crash, or do anything that looks like a crash.

6c2d233e7a8993d90fd97703ed8331f2d6e80671 introduced this method. You could tell from the deleted code that it was trying to replace abort() call. MHO is that report_fatal_error should crash (or could since crash is its default behavior, it could be overriden) as its name suggests. It is for shortcutting error propagation where it is hard to assume the compilation process could exit cleanly. So exit may not be a good choice.

I should never see a backtrace after calling report_fatal_error, and it should be a clean exit.

This part is customized by tools using install_fatal_error_handler (clang does). llc uses InitLLVM which asks for a backtrace.

For example I should not see a backtrace on a legalization failure with -global-isel-abort=1

I think you mean in this case, the crash better not to trigger a backtrace? The option name suggests it should abort.

I think there are 3 levels of error handling:

1. User facing errors that can occur due to user error, that should use the LLVMContext error reporting
2. Clean errors that generally should generally not be end user facing, but also should be handled gracefully and in all builds. Most cases that can only be produced by writing unrealistic MIR tests should use this form of error since it's not really worth the effort to produce a user facing error
3. Asserts for situations that should never happen. This is what llvm_unreachable/assert, and can be deleted in release builds.

This patch blobs uses 2 and 3 together. report_fatal_error is not equivalent to an assert.

Addressed header file order review comment

Addressed review comments.
Cosmetic test improvements.

Avoided use of ,unique, assembly feature when not using the integrated assembler.

Corrected silly mistake.

Added llvm_unreachable to getSectionPrefixForGlobal()

Rebased and removed changes to MCContext::renameELFSection().

Addressed review comments and rebased onto master.

In D72194#1861171, @MaskRay wrote:

I saw this a couple of weeks ago. Apologies that I did not pay enough attention. (I complained that GNU as lacks features. Things are changing (I'm glad that my __patchable_function_entries complaints worked:) )

In D73999#1866680, @psmith wrote:

In D73999#1866168, @MaskRay wrote:

Address review comments.

I tend to agree with Alan Modra's arguement in
https://sourceware.org/ml/binutils/2020-02/msg00093.html

I don't think so. User assembly often gets section attributes wrong
or leaves them off entirely for special sections known to the
assembler. ie. the first .section .foo above is a built-in rather
than user input.

Add more folks for feedback.

Please see my binutils post. For a .section directive with the same name but a different field. If the field is:

• sh_flags or sh_type: warn
• sh_link due to SHF_LINK_ORDER: no warning. produce separate sections
• sh_entsize due to SHF_MERGE: still controversial

I agree with warning for sh_flags, I think there is too much legacy code out there that would behave differently.
I agree with sh_link producing separate sections. In an ideal world no-one writes this by hand in assembly.
For sh_entsize SHF_MERGE, this is informing the linker that it can produce an optimisation, which it is permitted to ignore. I tend towards an error message if it is implementable as it is a strong message to fix the assembler. The next best thing is a warning then clearing SHF_MERGE and setting sh_entsize to 0. I don't think a warning on its own is safe.

In D74006#1860074, @MaskRay wrote:

Another thing is whether we should use sh_flags/sh_type to differentiate sections. In addition, whether we should use sh_entsize.
If yes, the implementation can be a bit clumsy. We may have to add many Elf*_Shdr fields to ELFSectionKey.
If no (GNU as behavior), we should add warnings. D73999 will do that.

The section group name is part of the key in both MC and GNU as. By analogy, SHF_LINK_ORDER+sh_link should probably be part of the key.

Field not specified by the .section directive (sh_addrline,sh_addr,sh_offset,sh_info) should definitely not be part of the key.

I have a slight preference for "no", though I don't currently have a particular good reason why "sh_entsize" should not be part of a key. Maybe we are fine with current .rodata.cst16 naming.

I think that this patch removes the need for uniquing sections for symbols with associated symbols which are explicitly assigned to a section name e.g. via attribute((section name)).

updated the patch and description

I have updated the patch. It now handles both "default" sections and "implicit" sections ("implicit" sections are those that are created by MC normally as it encounters symbols). I have improved the test coverage, organised the tests and improved the comments for the test cases. Sorry it took a while to update the patch.. this was a more difficult change than I had anticipated. I have tried a few different approaches, I am not entirely happy with the current implementation but it is difficult to simply further.

In D72197#1825494, @MaskRay wrote:

In D72197#1825390, @pcc wrote:

In D72197#1825247, @MaskRay wrote:

In D72197#1825012, @pcc wrote:

It looks like this change caused us to start rejecting:

.thumb
.thumb_func
.globl foo
.hidden foo
foo:
adr lr, foo + 1

with:

test.s:6:1: error: unsupported relocation on symbol
adr lr, foo + 1
^

This is exposed by this Android ART code: https://cs.android.com/android/platform/superproject/+/master:art/runtime/arch/arm/quick_entrypoints_arm.S;l=1826

I see a couple of possible fixes for this:

1. We could go back to the previous behaviour for global hidden symbols.
2. We could teach MC and LLD about the R_ARM_THM_ALU_PREL_11_0 relocation required to relocate this instruction. Arguably the ART code shouldn't be adding the 1 for Thumb here (because R_ARM_THM_ALU_PREL_11_0 adds the 1 itself), so ART would then need to be fixed.

foo is a hidden definition in the current object file. Can the ART code be changed to use a local symbol (adr lr, .Lfoo) instead?

It could, but I don't really see a good argument for local symbols having different behaviour to strong hidden symbols.

Not special casing hidden in the general case has the nice property that: the assembler behavior is not affected by the visibility. Only the binding can.

We can thus postpone decisions to the linker, and all the logic can be implemented on the linker side. Both a hidden defined symbol and a local symbol is considered non-preemptive, thus they have the same behavior at the linker stage.

The downside is that we increase object file sizes. Hidden symbols are not that common so the downside may be acceptable.

I rebased the patch and addressed the review comments. I also noticed that there was a problem with the patch when explicitly placing non-mergeable sections into one of the "default" mergeable sections (e.g. .debug_str). As these "default" mergeable sections were not being added to the map it wasn't possible to explicitly place symbols into these even if the symbols entsize was compatible (as they would be put into a uniqued section instead). This is not acceptable for these "default" sections since for some of these like e.g. .debug_str, current binutils consumers make the assumption that there can only be one of these sections. The fix for this is WIP and I haven't added LIT tests for it yet; but, I have put up the patch in WIP state to get comments on the approach taken.

In D72194#1821072, @smeenai wrote:

Is this worth merging into 10.0 if it lands after the branch?

Thanks, I'll address the points that you have raised.
I tried updating the assert and rebasing the patch but the assert is now firing in the It tests. I will have to investigate this tomorrow and update the patch then.

In D72194#1818766, @nickdesaulniers wrote:

This is a partial fix for

Is this still considered a partial fix? Seems pretty complete to me, otherwise what's still missing?

In D68101#1806475, @nickdesaulniers wrote:

In D68101#1806213, @rjmccall wrote:

In D68101#1806135, @nickdesaulniers wrote:

In D68101#1802280, @rjmccall wrote:

I laid out a series of three options before:

• Emit different object-file sections for different mergeability settings.
• Only mark an object-file section as mergeable if all the symbols in it would have the same mergeability settings.
• Stop implicitly using mergeability for "ordinary" sections (i.e. sections other than the string section).

Of the above, I really think #2 is the only complete solution.

Can you explain why you think #1 is not "complete"? All three seem to establish correctness; I can see how giving up on the optimization (#3) is not a great solution, but #1 doesn't have that problem and actually preserves it in more places. To be clear, this is just taking advantage of the ability to have multiple sections with the same name in an ELF object file; they will still be assembled into a single section in the linked image.

My understanding is that changing the flags on an MCSection retroactively is pretty counter to the architecture, so I'm not sure how we'd actually achieve #2.

Ah, ok, I reread https://reviews.llvm.org/D72194 and see that it's creating non-contiguous sections (option 1), with unique entity sizes. That should be fine. Dissenting opinion retracted. We should prefer https://reviews.llvm.org/D72194 with the commit message updated.

Updated the diff to address review comments and expand test coverage.

In D72197#1806225, @MaskRay wrote:

In D72197#1805438, @bd1976llvm wrote:

I wonder if is this really a bug or intentional. Clang and LLVM seem to ignore the possibility of symbol interposition (see: -fno-semantic-interposition). What is the LLVM policy for this?

Also, this code is missing a case for STB_GLOBAL and visibility != STV_DEFAULT. Actually, I have noticed before that this case is missing in various places in MC. As it stands this patch may cause a performance regression on platforms that use -fvisibility=hidden.

Visibility is irrelevant here. For a symbol defined in the same section as the instruction, the relocation record will reference a non-SECTION symbol instead of a STT_SECTION after this change. If the symbol is STV_HIDDEN, the linker will think the symbol is non-preemptible. So, no performance regression.

Thanks for the feedback. I'm sorry that I am being slow on this... v.busy at the moment. Will try to finish the patch in the next few days.

I wonder if is this really a bug or intentional. Clang and LLVM seem to ignore the possibility of symbol interposition (see: -fno-semantic-interposition). What is the LLVM policy for this?

In D68101#1802280, @rjmccall wrote:

The solution described in that comment is not acceptable. We are not going to tell users that they cannot assign symbols explicitly to sections because LLVM is unable to promise not to miscompile if they do. It is LLVM's responsibility to correctly compile valid code; enabling mergeability for a section containing unnamed_addr symbols is an optimization, and if it is not safe, it needs to be disabled until we can figure out a way to make it safe.

Sorry for the delay in updating this.

In D68101#1766359, @rjmccall wrote:

I can't speak for the pragma authors, but I strongly doubt the pragma is intended to force all affected globals to go into a single section unit, since the division into section units is purely an object-file representation issue.

The feature certainly could be implemented for COFF, so I think changing the name of the command line option is reasonable. The strings in the .deplibs sections map to libraries in an implementation defined manner, so I refer to them as "specifiers". In COFF dependent libraries are specified via "directives" in the object files. It might be worth naming the option something like: --dependent-lib-directives (as directives works for the entries in an ELF .deplibs sections equally as well as specifiers) ?