Bug 26262 - ld/plugin.c:plugin_notice may incorrectly replace an LTO bfd_link_hash_defined with bfd_link_hash_undefweak
Summary: ld/plugin.c:plugin_notice may incorrectly replace an LTO bfd_link_hash_define...
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.35
: P2 normal
Target Milestone: 2.35
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on: 26267
Blocks:
  Show dependency treegraph
 
Reported: 2020-07-18 23:56 UTC by Fangrui Song
Modified: 2020-07-22 12:51 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2020-07-19 00:00:00


Attachments
A patch (633 bytes, patch)
2020-07-20 13:03 UTC, H.J. Lu
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fangrui Song 2020-07-18 23:56:26 UTC
This can be reproduced with clang 10 (likely also 9; and all master commits before https://reviews.llvm.org/D83967 ). LLVMgold.so is needed.

% cat a.c
int main() {}
% /tmp/Debug/bin/clang -fuse-ld=bfd -fprofile-generate -flto a.c -Wl,-plugin-opt=save-temps -Wl,-y,__llvm_profile_raw_version -Wl,-plugin-opt=save-temps
/usr/bin/ld.bfd: /tmp/a-e575a9.o (symbol from plugin): definition of __llvm_profile_raw_version
/usr/bin/ld.bfd: /tmp/Debug/lib/clang/12.0.0/lib/linux/libclang_rt.profile-x86_64.a(InstrProfiling.c.o): definition of __llvm_profile_raw_version

/tmp/a-e575a9.o (symbol from plugin) provides a definition in a prevailing comdat group.
libclang_rt.profile-x86_64.a(InstrProfiling.c.o) is an weak definition which should not override the IR definition.

% cat a.out.resolution.txt 
/tmp/a-e575a9.o
-r=/tmp/a-e575a9.o,main,plx
-r=/tmp/a-e575a9.o,__llvm_profile_raw_version,l  # should be 'plx' instead of 'l'
-r=/tmp/a-e575a9.o,__llvm_profile_filename,plx

Gold and LLD are correct.

% /tmp/Debug/bin/clang -fuse-ld=gold -fprofile-generate -flto a.c -Wl,-plugin-opt=save-temps -Wl,-y,__llvm_profile_raw_version
/tmp/a-668c9c.o: definition of __llvm_profile_raw_version
/tmp/Debug/lib/clang/12.0.0/lib/linux/libclang_rt.profile-x86_64.a(InstrProfiling.c.o): definition of __llvm_profile_raw_version
a.out.o: definition of __llvm_profile_raw_version
% cat a.out.resolution.txt 
/tmp/a-668c9c.o
-r=/tmp/a-668c9c.o,main,plx
-r=/tmp/a-668c9c.o,__llvm_profile_raw_version,plx
-r=/tmp/a-668c9c.o,__llvm_profile_filename,plx

% /tmp/Debug/bin/clang -fuse-ld=lld -fprofile-generate -flto a.c -Wl,-plugin-opt=save-temps -Wl,-y,__llvm_profile_raw_version
/tmp/a-afb841.o: definition of __llvm_profile_raw_version
<internal>: reference to __llvm_profile_raw_version
lto.tmp: definition of __llvm_profile_raw_version
% cat a.out.resolution.txt
/tmp/a-afb841.o
-r=/tmp/a-afb841.o,main,plx
-r=/tmp/a-afb841.o,__llvm_profile_raw_version,plx
-r=/tmp/a-afb841.o,__llvm_profile_filename,plx


The ld bug is in ld/plugin.c:plugin_notice . ld somehow drops bfd_link_hash_defined (provided by "/tmp/a-e575a9.o (symbol from plugin)") in favor of bfd_link_hash_undefweak. Then bfd_link_hash_undefweak is overridden by the weak definition in libclang_rt.profile-x86_64.a(InstrProfiling.c.o)

      /* Otherwise, it must be a new def.
	 Ensure any symbol defined in an IR dummy BFD takes on a
	 new value from a real BFD.  Weak symbols are not normally
	 overridden by a new weak definition, and strong symbols
	 will normally cause multiple definition errors.  Avoid
	 this by making the symbol appear to be undefined.  */
      else if (((h->type == bfd_link_hash_defweak
		 || h->type == bfd_link_hash_defined)
		&& is_ir_dummy_bfd (sym_bfd = h->u.def.section->owner))
	       || (h->type == bfd_link_hash_common
		   && is_ir_dummy_bfd (sym_bfd = h->u.c.p->section->owner)))
	{
	  h->type = bfd_link_hash_undefweak;
	  h->u.undef.abfd = sym_bfd;
	}

The intended behavior is to let "/tmp/a-e575a9.o (symbol from plugin)" provide the definition.
Comment 1 H.J. Lu 2020-07-19 12:25:08 UTC
clang has different behavior depending on -fuse-ld=XXX:

[hjl@gnu-cfl-2 pr26262]$ clang -fuse-ld=gold -Wl,-y,__llvm_profile_raw_version -Wl,-plugin-opt=save-temps -fprofile-generate -flto -o a a.c -save-temps
a.o: definition of __llvm_profile_raw_version
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.profile-x86_64.a(InstrProfiling.c.o): definition of __llvm_profile_raw_version
a.o: definition of __llvm_profile_raw_version
[hjl@gnu-cfl-2 pr26262]$ clang -fuse-ld=bfd -Wl,-y,__llvm_profile_raw_version -Wl,-plugin-opt=save-temps -fprofile-generate -flto -o a a.o
/usr/bin/ld.bfd: a.o: definition of __llvm_profile_raw_version
[hjl@gnu-cfl-2 pr26262]$ clang -fuse-ld=bfd -Wl,-y,__llvm_profile_raw_version -Wl,-plugin-opt=save-temps -fprofile-generate -flto -o a a.c -save-temps
/usr/bin/ld.bfd: a.o (symbol from plugin): definition of __llvm_profile_raw_version
/usr/bin/ld.bfd: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.profile-x86_64.a(InstrProfiling.c.o): definition of __llvm_profile_raw_version
[hjl@gnu-cfl-2 pr26262]$ clang -fuse-ld=gold -Wl,-y,__llvm_profile_raw_version -Wl,-plugin-opt=save-temps -fprofile-generate -flto -o a a.o
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.profile-x86_64.a(InstrProfiling.c.o): definition of __llvm_profile_raw_version
[hjl@gnu-cfl-2 pr26262]$ rm a.o
[hjl@gnu-cfl-2 pr26262]$ clang -fprofile-generate -flto -c a.c
[hjl@gnu-cfl-2 pr26262]$ clang -fuse-ld=bfd -Wl,-y,__llvm_profile_raw_version -Wl,-plugin-opt=save-temps -fprofile-generate -flto -o a a.o
/usr/bin/ld.bfd: a.o (symbol from plugin): definition of __llvm_profile_raw_version
/usr/bin/ld.bfd: /usr/lib64/clang/10.0.0/lib/linux/libclang_rt.profile-x86_64.a(InstrProfiling.c.o): definition of __llvm_profile_raw_version
[hjl@gnu-cfl-2 pr26262]$ clang -fuse-ld=gold -Wl,-y,__llvm_profile_raw_version -Wl,-plugin-opt=save-temps -fprofile-generate -flto -o a a.o
/usr/lib64/clang/10.0.0/lib/linux/libclang_rt.profile-x86_64.a(InstrProfiling.c.o): definition of __llvm_profile_raw_version
[hjl@gnu-cfl-2 pr26262]$ 

It looks like a clang bug.
Comment 2 Fangrui Song 2020-07-20 02:21:51 UTC
(In reply to H.J. Lu from comment #1)
> clang has different behavior depending on -fuse-ld=XXX:
> 
> ...
> 
> It looks like a clang bug.

Thanks for the report. It is an LLVMgold.so bug. LLVMgold.so is used by GNU ld and gold for clang LTO (full LTO or ThinLTO). The problem is that

clang -fuse-ld=bfd -flto a.o -o a -Wl,-plugin-opt=save-temps  # override a.o

I will fix it in https://reviews.llvm.org/D84132 and cherry pick it into llvm-project's release/11.x branch (newly created for the 11.0.0 release).

Let's move back to the original topic. Use a different output filename (than the basename of the source file) to avoid the -plugin-opt=save-temps issue.

-----

% clang -fuse-ld=bfd -fprofile-generate=out -flto a.c -Wl,-y,__llvm_profile_raw_version -Wl,-plugin-opt=save-temps -o aa
/usr/bin/ld.bfd: /tmp/a-1cd442.o (symbol from plugin): definition of __llvm_profile_raw_version
/usr/bin/ld.bfd: /tmp/RelA/lib/clang/12.0.0/lib/linux/libclang_rt.profile-x86_64.a(InstrProfiling.c.o): definition of __llvm_profile_raw_version
% ./aa
% llvm-profdata merge out -o a.profdata
% llvm-profdata show a.profdata  
Instrumentation level: Front-end    ### incorrect, it should be IR
Total functions: 1
Maximum function count: 1
Maximum internal block count: 0

% rm -r out
% clang -fuse-ld=gold -fprofile-generate=out -flto a.c -Wl,-y,__llvm_profile_raw_version -Wl,-plugin-opt=save-temps -o aa
/tmp/a-3b2969.o: definition of __llvm_profile_raw_version
/tmp/RelA/lib/clang/12.0.0/lib/linux/libclang_rt.profile-x86_64.a(InstrProfiling.c.o): definition of __llvm_profile_raw_version
aa.o: definition of __llvm_profile_raw_version
% ./aa
% llvm-profdata merge out -o a.profdata
% llvm-profdata show a.profdata  
Instrumentation level: IR
Total functions: 1
Maximum function count: 1
Maximum internal block count: 0

----

As my first comment says, you can inspect aa.resolution.txt to see that GNU ld incorrectly thinks that __llvm_profile_raw_versio is non-prevailing.
Comment 3 H.J. Lu 2020-07-20 13:03:12 UTC
Created attachment 12713 [details]
A patch
Comment 4 H.J. Lu 2020-07-20 13:19:57 UTC
[hjl@gnu-cfl-2 pr26262]$ cat pr26262a.c
#include <stdio.h>

int counter;
extern void foo (void);
extern void bar (void);
extern void xxx (void);

int
main(void)
{
  bar ();
  foo ();
  xxx ();
  if (counter == 1)
    printf ("PASS\n");
  return 0;
}
[hjl@gnu-cfl-2 pr26262]$ cat pr26262b.c
__attribute__ ((noinline))
void
bar (void)
{
}
[hjl@gnu-cfl-2 pr26262]$ cat pr26262c.c
#include <stdlib.h>

extern int counter;

void
foo (void)
{
  counter++;
}

__attribute__((weak))
void
bar (void)
{
  abort ();
}
[hjl@gnu-cfl-2 pr26262]$ cat pr26262d.c
extern void bar (void);
void
xxx (void)
{
  bar ();
}
[hjl@gnu-cfl-2 pr26262]$ make
gcc -O2 -flto   -c -o pr26262a.o pr26262a.c
gcc -O2 -flto   -c -o pr26262b.o pr26262b.c
gcc -O2   -c -o pr26262c.o pr26262c.c
gcc -O2   -c -o pr26262d.o pr26262d.c
gcc -fuse-ld=bfd  -O2 -o x pr26262a.o pr26262b.o pr26262c.o pr26262d.o
./x
make: *** [Makefile:11: all] Aborted (core dumped)
[hjl@gnu-cfl-2 pr26262]$
Comment 5 H.J. Lu 2020-07-20 13:35:53 UTC
A patch is posted at

https://sourceware.org/pipermail/binutils/2020-July/112454.html
Comment 6 H.J. Lu 2020-07-20 15:51:34 UTC
(In reply to H.J. Lu from comment #5)
> A patch is posted at
> 
> https://sourceware.org/pipermail/binutils/2020-July/112454.html

Fix isn't complete, see PR 26267.
Comment 7 Sourceware Commits 2020-07-22 10:59:14 UTC
The master branch has been updated by H.J. Lu <hjl@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=0e6a3f07f50723d1831291492b96fdf74bcbdc11

commit 0e6a3f07f50723d1831291492b96fdf74bcbdc11
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Wed Jul 22 03:49:07 2020 -0700

    ld: Properly override the IR definition
    
    We change the previous definition in the IR object to undefweak only
    after all LTO symbols have been read.
    
    include/
    
            PR ld/26262
            PR ld/26267
            * bfdlink.h (bfd_link_info): Add lto_all_symbols_read.
    
    ld/
    
            PR ld/26262
            PR ld/26267
            * ldlang.c (lang_process): Set lto_all_symbols_read after all
            LTO IR symbols have been read.
            * plugin.c (plugin_notice): Override the IR definition only if
            all LTO IR symbols have been read or the new definition is
            non-weak and the the IR definition is weak
            * testsuite/ld-plugin/lto.exp: Run PR ld/26262 and ld/26267
            tests.
            * testsuite/ld-plugin/pr26262a.c: New file.
            * testsuite/ld-plugin/pr26262b.c: Likewise.
            * testsuite/ld-plugin/pr26262c.c: Likewise.
            * testsuite/ld-plugin/pr26267.err: Likewise.
            * testsuite/ld-plugin/pr26267a.c: Likewise.
            * testsuite/ld-plugin/pr26267b.c: Likewise.
            * testsuite/ld-plugin/pr26267c.c: Likewise.
Comment 8 H.J. Lu 2020-07-22 12:30:12 UTC
Fixed for 2.36 so far.
Comment 9 Sourceware Commits 2020-07-22 12:50:59 UTC
The binutils-2_35-branch branch has been updated by H.J. Lu <hjl@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=041d1c2d4f03a134cb7574e4d62d2ce4418503ff

commit 041d1c2d4f03a134cb7574e4d62d2ce4418503ff
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Wed Jul 22 03:49:07 2020 -0700

    ld: Properly override the IR definition
    
    We change the previous definition in the IR object to undefweak only
    after all LTO symbols have been read.
    
    include/
    
            PR ld/26262
            PR ld/26267
            * bfdlink.h (bfd_link_info): Add lto_all_symbols_read.
    
    ld/
    
            PR ld/26262
            PR ld/26267
            * ldlang.c (lang_process): Set lto_all_symbols_read after all
            LTO IR symbols have been read.
            * plugin.c (plugin_notice): Override the IR definition only if
            all LTO IR symbols have been read or the new definition is
            non-weak and the the IR definition is weak
            * testsuite/ld-plugin/lto.exp: Run PR ld/26262 and ld/26267
            tests.
            * testsuite/ld-plugin/pr26262a.c: New file.
            * testsuite/ld-plugin/pr26262b.c: Likewise.
            * testsuite/ld-plugin/pr26262c.c: Likewise.
            * testsuite/ld-plugin/pr26267.err: Likewise.
            * testsuite/ld-plugin/pr26267a.c: Likewise.
            * testsuite/ld-plugin/pr26267b.c: Likewise.
            * testsuite/ld-plugin/pr26267c.c: Likewise.
    
    (cherry picked from commit 0e6a3f07f50723d1831291492b96fdf74bcbdc11)
Comment 10 H.J. Lu 2020-07-22 12:51:35 UTC
Fixed for 2.35.