Bug 22868 - Gold does not select proper symbol visibility when used with LLVM gold-plugin
Summary: Gold does not select proper symbol visibility when used with LLVM gold-plugin
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gold (show other bugs)
Version: 2.31
: P2 normal
Target Milestone: ---
Assignee: Cary Coutant
URL:
Keywords:
: 22994 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-02-20 14:59 UTC by georgerim
Modified: 2018-03-31 12:21 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2018-03-22 00:00:00


Attachments
Recording (891 bytes, application/x-xz)
2018-03-26 07:56 UTC, Nick Clifton
Details
Requested record file (576 bytes, application/x-tar)
2018-03-26 08:05 UTC, georgerim
Details

Note You need to log in before you can comment on or make changes to this bug.
Description georgerim 2018-02-20 14:59:48 UTC
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
Comment 1 Michael Matz 2018-03-05 21:29:19 UTC
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.
Comment 2 Shea Levy 2018-03-19 14:46:13 UTC
Is anything significant likely to break with the patch reverted locally for now?
Comment 3 Cary Coutant 2018-03-22 16:45:04 UTC
*** Bug 22994 has been marked as a duplicate of this bug. ***
Comment 4 Michael Matz 2018-03-22 16:49:27 UTC
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;
Comment 5 Cary Coutant 2018-03-22 19:24:57 UTC
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?
Comment 6 georgerim 2018-03-23 09:01:42 UTC
(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
Comment 7 georgerim 2018-03-23 09:07:04 UTC
For above case, a.o and b.o objects have symbols f1 with different visibility.
Comment 8 Cary Coutant 2018-03-23 17:14:43 UTC
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.
Comment 9 Nick Clifton 2018-03-26 07:56:26 UTC
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
Comment 10 georgerim 2018-03-26 08:05:24 UTC
Created attachment 10916 [details]
Requested record file

And here is mine record file for this bug.
Comment 11 cvs-commit@gcc.gnu.org 2018-03-26 17:55:29 UTC
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.
Comment 12 Cary Coutant 2018-03-26 17:56:42 UTC
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.
Comment 13 cvs-commit@gcc.gnu.org 2018-03-28 16:16:23 UTC
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.