Page MenuHomePhabricator

bd1976llvm (ben)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2017, 9:47 AM (191 w, 4 d)

Recent Activity

Wed, Sep 16

bd1976llvm added a comment to D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.

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

Wed, Sep 16, 7:28 AM · Restricted Project

Tue, Sep 15

bd1976llvm updated subscribers of D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.

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

Tue, Sep 15, 1:28 AM · Restricted Project

Fri, Aug 28

bd1976llvm requested review of D86828: [windows-itanium] make dllimport/export handling closer to MS behavior.
Fri, Aug 28, 6:35 PM

Thu, Aug 27

bd1976llvm added a comment to D83069: [lit] warn if explicitly specified test won't be run indirectly.

ping!

Thu, Aug 27, 3:49 AM · Restricted Project

Tue, Aug 25

bd1976llvm added a comment to D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.

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.

Tue, Aug 25, 1:37 PM · Restricted Project
bd1976llvm added a comment to D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.

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

Tue, Aug 25, 8:48 AM · Restricted Project

Aug 14 2020

bd1976llvm added a comment to D83069: [lit] warn if explicitly specified test won't be run indirectly.

ping!

Aug 14 2020, 3:55 AM · Restricted Project

Aug 12 2020

bd1976llvm added a comment to D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.

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.

Aug 12 2020, 9:05 AM · Restricted Project
bd1976llvm updated the summary of D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.
Aug 12 2020, 1:38 AM · Restricted Project

Aug 11 2020

bd1976llvm updated the diff for D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.

Moved testing into linux-preemption.ll

Aug 11 2020, 6:03 PM · Restricted Project
bd1976llvm updated the summary of D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.
Aug 11 2020, 6:00 PM · Restricted Project
bd1976llvm added a comment to D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.

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.

Aug 11 2020, 2:55 PM · Restricted Project
bd1976llvm added a comment to D73230: [X86][ELF] Prefer to lower MC_GlobalAddress operands to .Lfoo$local.

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

Aug 11 2020, 2:15 PM · Restricted Project
bd1976llvm updated the summary of D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.
Aug 11 2020, 2:14 PM · Restricted Project
bd1976llvm requested review of D85782: [X86][ELF] Prefer lowering MC_GlobalAddress operands to .Lfoo$local only for STV_DEFAULT globals.
Aug 11 2020, 2:10 PM · Restricted Project
bd1976llvm added a comment to D73230: [X86][ELF] Prefer to lower MC_GlobalAddress operands to .Lfoo$local.

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

Aug 11 2020, 12:29 PM · Restricted Project

Aug 10 2020

bd1976llvm added a comment to D73230: [X86][ELF] Prefer to lower MC_GlobalAddress operands to .Lfoo$local.

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

Aug 10 2020, 4:58 PM · Restricted Project
bd1976llvm added a comment to D73230: [X86][ELF] Prefer to lower MC_GlobalAddress operands to .Lfoo$local.

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

Aug 10 2020, 11:56 AM · Restricted Project
bd1976llvm added a comment to D73230: [X86][ELF] Prefer to lower MC_GlobalAddress operands to .Lfoo$local.

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

Aug 10 2020, 11:16 AM · Restricted Project
bd1976llvm added a comment to D73230: [X86][ELF] Prefer to lower MC_GlobalAddress operands to .Lfoo$local.

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

Aug 10 2020, 10:44 AM · Restricted Project
bd1976llvm added a comment to D73230: [X86][ELF] Prefer to lower MC_GlobalAddress operands to .Lfoo$local.

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

Aug 10 2020, 9:16 AM · Restricted Project
bd1976llvm added a comment to D73230: [X86][ELF] Prefer to lower MC_GlobalAddress operands to .Lfoo$local.

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

Aug 10 2020, 4:02 AM · Restricted Project

Aug 3 2020

bd1976llvm added inline comments to D83530: [llvm-symbolizer] Switch command line parsing from llvm::cl to OptTable.
Aug 3 2020, 6:38 AM · Restricted Project

Jul 20 2020

bd1976llvm added inline comments to D83069: [lit] warn if explicitly specified test won't be run indirectly.
Jul 20 2020, 9:16 PM · Restricted Project

Jul 7 2020

bd1976llvm updated the diff for D83272: [LLD][ELF][Windows] small improvement to D82567.

Addressed review comments.

Jul 7 2020, 6:48 AM · Restricted Project
bd1976llvm added inline comments to D83272: [LLD][ELF][Windows] small improvement to D82567.
Jul 7 2020, 6:48 AM · Restricted Project
bd1976llvm added a comment to D83272: [LLD][ELF][Windows] small improvement to D82567.

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

Jul 7 2020, 6:30 AM · Restricted Project
bd1976llvm updated the diff for D83272: [LLD][ELF][Windows] small improvement to D82567.

Share logic between the two code paths.

Jul 7 2020, 6:29 AM · Restricted Project

Jul 6 2020

bd1976llvm created D83272: [LLD][ELF][Windows] small improvement to D82567.
Jul 6 2020, 6:10 PM · Restricted Project

Jul 3 2020

bd1976llvm added a comment to D83069: [lit] warn if explicitly specified test won't be run indirectly.

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?

Jul 3 2020, 2:07 AM · Restricted Project

Jul 2 2020

bd1976llvm updated the diff for D82567: [LLD][ELF][Windows] Allow LLD to overwrite existing output files that are in use.

Thanks for the suggested improvements.

Jul 2 2020, 11:51 AM · Restricted Project
bd1976llvm created D83069: [lit] warn if explicitly specified test won't be run indirectly.
Jul 2 2020, 11:51 AM · Restricted Project

Jul 1 2020

bd1976llvm updated the diff for D82567: [LLD][ELF][Windows] Allow LLD to overwrite existing output files that are in use.

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

Jul 1 2020, 1:02 AM · Restricted Project

Jun 30 2020

bd1976llvm updated the diff for D82567: [LLD][ELF][Windows] Allow LLD to overwrite existing output files that are in use.

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

Jun 30 2020, 1:03 PM · Restricted Project
bd1976llvm updated the diff for D82542: [Support][Windows] Prevent 2s delay when renaming a file that does not exist.

Addressed review comments.

Jun 30 2020, 3:46 AM · Restricted Project
bd1976llvm added inline comments to D82542: [Support][Windows] Prevent 2s delay when renaming a file that does not exist.
Jun 30 2020, 3:46 AM · Restricted Project

Jun 25 2020

bd1976llvm created D82567: [LLD][ELF][Windows] Allow LLD to overwrite existing output files that are in use.
Jun 25 2020, 9:07 AM · Restricted Project
bd1976llvm created D82542: [Support][Windows] Prevent 2s delay when renaming a file that does not exist.
Jun 25 2020, 5:15 AM · Restricted Project

May 14 2020

bd1976llvm created D79980: [PS4] Enable relaxed relocations by default.
May 14 2020, 7:35 PM · Restricted Project

Apr 16 2020

bd1976llvm committed rG86478d3de91a: [MC][ELF] Put explicit section name symbols into entry size compatible sections (authored by bd1976llvm).
[MC][ELF] Put explicit section name symbols into entry size compatible sections
Apr 16 2020, 12:17 PM
bd1976llvm closed D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Apr 16 2020, 12:16 PM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

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.

Apr 16 2020, 11:08 AM · Restricted Project, Restricted Project

Apr 8 2020

bd1976llvm added a comment to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

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

Apr 8 2020, 2:08 PM · Restricted Project, Restricted Project
bd1976llvm added a comment to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

Thanks for the interdiff rationale. Please take another look!

Apr 8 2020, 7:03 AM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
  • 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.
Apr 8 2020, 7:02 AM · Restricted Project, Restricted Project

Apr 7 2020

bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

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

Apr 7 2020, 4:22 PM · Restricted Project, Restricted Project
bd1976llvm added a comment to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

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

Apr 7 2020, 4:22 PM · Restricted Project, Restricted Project

Apr 6 2020

bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

Report error using LLVM diagnostics framework.

Apr 6 2020, 3:49 PM · Restricted Project, Restricted Project

Apr 2 2020

bd1976llvm added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Apr 2 2020, 6:27 PM · Restricted Project, Restricted Project

Apr 1 2020

bd1976llvm added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Apr 1 2020, 6:32 PM · Restricted Project, Restricted Project

Mar 31 2020

bd1976llvm added a comment to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

@nickdesaulniers and @MaskRay,

Mar 31 2020, 12:33 PM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

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

Mar 31 2020, 12:33 PM · Restricted Project, Restricted Project
bd1976llvm added a comment to rG8cedf0e2994c: Reland "[Support] make report_fatal_error `abort` instead of `exit`".

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.

Mar 31 2020, 9:22 AM

Mar 30 2020

bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

Addressed header file order review comment

Mar 30 2020, 7:08 PM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

Addressed review comments.
Cosmetic test improvements.

Mar 30 2020, 7:08 PM · Restricted Project, Restricted Project
bd1976llvm added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Mar 30 2020, 7:08 PM · Restricted Project, Restricted Project

Mar 26 2020

bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

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

Mar 26 2020, 7:35 PM · Restricted Project, Restricted Project

Mar 25 2020

bd1976llvm added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Mar 25 2020, 6:25 PM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

Corrected silly mistake.

Mar 25 2020, 5:53 PM · Restricted Project, Restricted Project

Mar 23 2020

bd1976llvm added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Mar 23 2020, 11:28 AM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

Added llvm_unreachable to getSectionPrefixForGlobal()

Mar 23 2020, 11:28 AM · Restricted Project, Restricted Project

Mar 20 2020

bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

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

Mar 20 2020, 4:18 AM · Restricted Project, Restricted Project

Mar 19 2020

bd1976llvm added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Mar 19 2020, 1:09 PM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Mar 19 2020, 12:32 PM · Restricted Project, Restricted Project

Mar 17 2020

bd1976llvm updated the summary of D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Mar 17 2020, 8:31 AM · Restricted Project, Restricted Project
bd1976llvm updated the summary of D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Mar 17 2020, 8:31 AM · Restricted Project, Restricted Project
bd1976llvm updated the summary of D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Mar 17 2020, 8:31 AM · Restricted Project, Restricted Project

Mar 16 2020

bd1976llvm added a comment to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

Addressed review comments and rebased onto master.

Mar 16 2020, 9:14 AM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Mar 16 2020, 9:14 AM · Restricted Project, Restricted Project

Mar 10 2020

bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Mar 10 2020, 12:33 PM · Restricted Project, Restricted Project
bd1976llvm added a comment to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

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

Mar 10 2020, 12:00 PM · Restricted Project, Restricted Project
bd1976llvm updated the summary of D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Mar 10 2020, 9:12 AM · Restricted Project, Restricted Project

Feb 10 2020

bd1976llvm added a comment to D73999: [MC][ELF] Error for sh_type, sh_flags or sh_entsize change.

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.

Feb 10 2020, 9:32 AM · Restricted Project

Feb 5 2020

bd1976llvm added a comment to D74006: [MC][ELF] Make linked-to symbol name part of ELFSectionKey.

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.

Feb 5 2020, 11:02 AM · Restricted Project
bd1976llvm added a comment to D74006: [MC][ELF] Make linked-to symbol name part of ELFSectionKey.

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

Feb 5 2020, 5:13 AM · Restricted Project
bd1976llvm added reviewers for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes: MaskRay, maskray0.
Feb 5 2020, 5:04 AM · Restricted Project, Restricted Project
bd1976llvm updated the summary of D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Feb 5 2020, 5:04 AM · Restricted Project, Restricted Project

Feb 4 2020

bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

updated the patch and description

Feb 4 2020, 6:40 PM · Restricted Project, Restricted Project

Jan 23 2020

bd1976llvm added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Jan 23 2020, 6:49 PM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

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.

Jan 23 2020, 6:40 PM · Restricted Project, Restricted Project

Jan 16 2020

bd1976llvm added a comment to D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.
In D72197#1825390, @pcc 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.

Jan 16 2020, 5:00 PM · Restricted Project
bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

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.

Jan 16 2020, 4:23 PM · Restricted Project, Restricted Project
bd1976llvm added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Jan 16 2020, 3:03 AM · Restricted Project, Restricted Project
bd1976llvm added a comment to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

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

Jan 16 2020, 2:45 AM · Restricted Project, Restricted Project

Jan 14 2020

bd1976llvm added a comment to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

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.

Jan 14 2020, 6:10 PM · Restricted Project, Restricted Project
bd1976llvm added a comment to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

This is a partial fix for

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

Jan 14 2020, 12:21 PM · Restricted Project, Restricted Project
bd1976llvm added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Jan 14 2020, 12:12 PM · Restricted Project, Restricted Project

Jan 13 2020

bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

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.

Jan 13 2020, 5:36 PM · Restricted Project
bd1976llvm abandoned D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.
Jan 13 2020, 5:36 PM · Restricted Project
bd1976llvm updated the diff for D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.

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

Jan 13 2020, 5:36 PM · Restricted Project, Restricted Project
bd1976llvm added inline comments to D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Jan 13 2020, 5:28 PM · Restricted Project, Restricted Project
bd1976llvm retitled D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes from WIP alternative approach for D68101 to [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Jan 13 2020, 5:18 PM · Restricted Project, Restricted Project
bd1976llvm added a comment to D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.

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.

Jan 13 2020, 12:09 PM · Restricted Project

Jan 6 2020

bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

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.

Jan 6 2020, 7:47 PM · Restricted Project
bd1976llvm added a comment to D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local.

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?

Jan 6 2020, 2:08 AM · Restricted Project

Jan 3 2020

bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

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.

Jan 3 2020, 6:58 PM · Restricted Project
bd1976llvm created D72194: [MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes.
Jan 3 2020, 6:49 PM · Restricted Project, Restricted Project

Jan 2 2020

bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

Sorry for the delay in updating this.

Jan 2 2020, 4:59 PM · Restricted Project

Dec 3 2019

bd1976llvm added a comment to D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable.

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.

Dec 3 2019, 2:29 AM · Restricted Project

Dec 2 2019

bd1976llvm added a comment to D70665: [llvm-readobj] - Implement --dependent-libraries flag..

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

Dec 2 2019, 7:01 PM · Restricted Project