| Summary: | Gold does not select proper symbol visibility when used with LLVM gold-plugin | ||
|---|---|---|---|
| Product: | binutils | Reporter: | georgerim |
| Component: | gold | Assignee: | Cary Coutant <ccoutant> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | ian, jeremip11, matz, nickc, shea |
| Priority: | P2 | ||
| Version: | 2.31 | ||
| Target Milestone: | --- | ||
| Host: | Target: | ||
| Build: | Last reconfirmed: | 2018-03-22 00:00:00 | |
| Attachments: |
Recording
Requested record file |
||
The second testsuite fail in llvm has probably the same reason, but just for completeness: visiblity.ll fails. It essentially has a weak and a non-weak
definition of the same symbol:
one.ll:
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
define weak protected void @foo() {
ret void
}
two.ll:
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
define void @foo() {
ret void
}
But the linked result (with ld.gold):
Symbol {
Name: foo (44)
Value: 0x290
Size: 1
Binding: Weak (0x2)
Type: Function (0x2)
Other [ (0x3)
STV_PROTECTED (0x3)
]
Section: .text (0x5)
}
So the binding is weak. In ELF symbol resolution rules a non-weak definition overrides a weak one, so it should actually be global. (In this case the
STV_PROTECTED is correct)
ld.bfd gets this right.
Is anything significant likely to break with the patch reverted locally for now? *** Bug 22994 has been marked as a duplicate of this bug. *** FWIW, I'm using this patch in our binutils package:
----------------------------------------------
Fixes two testsuite fails in the gold plugin tests of LLVM.
Aka binutils/PR22868
Index: binutils-2.30/gold/resolve.cc
===================================================================
--- binutils-2.30.orig/gold/resolve.cc 2018-01-13 14:31:16.000000000 +0100
+++ binutils-2.30/gold/resolve.cc 2018-03-06 16:58:42.000000000 +0100
@@ -265,10 +265,13 @@ Symbol_table::resolve(Sized_symbol<size>
return;
// Likewise for an absolute symbol defined twice with the same value.
+ // plugin-symbols are always absolute with same value here, so ignore those
if (!is_ordinary
&& st_shndx == elfcpp::SHN_ABS
&& !to_is_ordinary
&& to_shndx == elfcpp::SHN_ABS
+ && object->pluginobj() == NULL
+ && to->object()->pluginobj() == NULL
&& to->value() == sym.get_st_value())
return;
I'm not able to reproduce this problem in my test framework. Whether or not we override the symbol when scanning the IR, we still pick up the visibility when processing the replacement files. Could it be the case that your replacement objects do not have the STV_PROTECTED visibility on the symbol? (In reply to Cary Coutant from comment #5) > I'm not able to reproduce this problem in my test framework. Whether or not > we override the symbol when scanning the IR, we still pick up the visibility > when processing the replacement files. > > Could it be the case that your replacement objects do not have the > STV_PROTECTED visibility on the symbol? Not sure what do you mean by "replacement object"? But in my case (1st post), the order of objects in the command line affected the visibility. I also tested the attachment from Bug 22994 today and issue reproduced for me with the latest gold sources: ~/binutils-2.30-branch/binutils-gdb/build/gold/ld-new -v GNU gold (GNU Binutils 2.30.51.20180323) 1.15 ~/binutils-2.30-branch/binutils-gdb/build/gold/ld-new -shared -o c.o -plugin /home/umb/LLVM/build_goldplugin/lib/LLVMgold.so a.o b.o -m elf_x86_64 -plugin-opt=save-temps readelf --syms c.o | grep f1 18: 00000000000005b0 6 FUNC WEAK DEFAULT 8 f1 2: 00000000000005c0 2 FUNC LOCAL DEFAULT 8 f1.2 24: 00000000000005b0 6 FUNC WEAK DEFAULT 8 f1 ~/binutils-2.30-branch/binutils-gdb/build/gold/ld-new -shared -o c.o -plugin /home/umb/LLVM/build_goldplugin/lib/LLVMgold.so b.o a.o -m elf_x86_64 -plugin-opt=save-temps readelf --syms c.o | grep f1 10: 0000000000000490 2 FUNC WEAK PROTECTED 6 f1 17: 0000000000000490 2 FUNC WEAK PROTECTED 6 f1 For above case, a.o and b.o objects have symbols f1 with different visibility. I've just committed a gold patch that adds a --debug=plugin option. This will create a temporary directory under your current working directory named "gold-record-XXXXX", and will store a log file and copies of the replacement objects generated by the LTO plugin. Please run your failing test case with this option (-Wl,--debug=plugin), and send me a tar file of the resulting directory. Created attachment 10915 [details] Recording Hi Cary, Here is a recording of the test case from PR 22994. I hope that it helps. Cheers Nick Created attachment 10916 [details]
Requested record file
And here is mine record file for this bug.
The master branch has been updated by Cary Coutant <ccoutant@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=0b7a4aa6ba5a144b7ce616e80e95d9ff944fec2e commit 0b7a4aa6ba5a144b7ce616e80e95d9ff944fec2e Author: Cary Coutant <ccoutant@gmail.com> Date: Fri Mar 23 09:03:34 2018 -0700 Fix case where IR file provides symbol visibility but replacement file does not. In PR 22868, two IR files provide conflicting visibility for a symbol. When a def with PROTECTED visibility is seen after a def with DEFAULT visibility, gold does not override the visibility. Later, if the replacement object define the symbol with DEFAULT visibility, the symbol remains DEFAULT. This was caused by a recent change to allow multiply-defined absolute symbols, combined with the fact that the plugin framework was using SHN_ABS as the section index for placeholder symbols. The solution is to use a real (but arbitrary) section index. gold/ PR gold/22868 * plugin.cc (Sized_pluginobj::do_add_symbols): Use a real section index instead of SHN_ABS for defined symbols. * testsuite/Makefile.am (plugin_pr22868): New test case. * testsuite/Makefile.in: Regenerate * testsuite/plugin_pr22868.sh: New test script. * testsuite/plugin_pr22868_a.c: New source file. * testsuite/plugin_pr22868_b.c: New source file. Thanks for the recordings! That helped me find a stupid bug in my own attempt to reproduce the problem. Fixed on trunk; will backport to 2.30. The binutils-2_30-branch branch has been updated by Cary Coutant <ccoutant@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=330b90b5ffbbc20c5de6ae6c7f60c40fab2e7a4f commit 330b90b5ffbbc20c5de6ae6c7f60c40fab2e7a4f Author: Cary Coutant <ccoutant@gmail.com> Date: Fri Mar 23 09:03:34 2018 -0700 Fix case where IR file provides symbol visibility but replacement file does not. In PR 22868, two IR files provide conflicting visibility for a symbol. When a def with PROTECTED visibility is seen after a def with DEFAULT visibility, gold does not override the visibility. Later, if the replacement object define the symbol with DEFAULT visibility, the symbol remains DEFAULT. This was caused by a recent change to allow multiply-defined absolute symbols, combined with the fact that the plugin framework was using SHN_ABS as the section index for placeholder symbols. The solution is to use a real (but arbitrary) section index. gold/ PR gold/22868 * plugin.cc (Sized_pluginobj::do_add_symbols): Use a real section index instead of SHN_ABS for defined symbols. * testsuite/Makefile.am (plugin_pr22868): New test case. * testsuite/Makefile.in: Regenerate * testsuite/plugin_pr22868.sh: New test script. * testsuite/plugin_pr22868_a.c: New source file. * testsuite/plugin_pr22868_b.c: New source file. |
Issue is following. If we have 2 IR files, where the same weak symbol has different visibility: comdat.ll: target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu" define weak_odr i32 @f1(i8*) { ret i32 42 } comdat_input.ll: target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu" define weak_odr protected i32 @f1(i8* %this) { ret i32 41 } Then symbol expected to be protected in output. Invocation used: llvm-as comdat.ll -o comdat.ll.tmp1.o llvm-as comdat_input.ll -o comdat.ll.tmp2.o ./ld.gold -shared -o out.gold -plugin <path>/LLVMgold.so comdat.ll.tmp1.o comdat.ll.tmp2.o -m elf_x86_64 But at fact now symbol is not protected: readelf -a out.gold ... Symbol { Name: f1 (44) Value: 0x290 Size: 6 Binding: Weak (0x2) Type: Function (0x2) Other: 0 Section: .text (0x5) } And if I switch order of objects to comdat.ll.tmp2.o comdat.ll.tmp1.o then it becomes protected: Symbol { Name: f1 (44) Value: 0x290 Size: 6 Binding: Weak (0x2) Type: Function (0x2) Other [ (0x3) STV_PROTECTED (0x3) ] Section: .text (0x5) } It is broken starting from following commit: http://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=5dc824ed42cd173c1525f5abc76f4091f11a4dbc Before commit above symbol always was STV_PROTECTED as expected. This was reported for LLVM initially here: https://bugs.llvm.org/show_bug.cgi?id=36166