Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lld] Errors about duplicate gcov symbols after 8f91f38 #48570

Closed
llvmbot opened this issue Feb 17, 2021 · 8 comments
Closed

[lld] Errors about duplicate gcov symbols after 8f91f38 #48570

llvmbot opened this issue Feb 17, 2021 · 8 comments
Labels
bugzilla Issues migrated from bugzilla lld:ELF

Comments

@llvmbot
Copy link
Member

llvmbot commented Feb 17, 2021

Bugzilla Link 49226
Resolution FIXED
Resolved on Jun 24, 2021 02:57
Version unspecified
OS Windows NT
Blocks #48661
Reporter LLVM Bugzilla Contributor
CC @abpostelnicu,@MaskRay,@marco-c,@glandium,@nikic,@smithp35,@tstellar

Extended Description

https://treeherder.mozilla.org/logviewer?job_id=330246994&repo=try&lineNumber=59045

When linking rust code compiled with coverage enabled, we see error: duplicate symbol: __gcov_dump and similar.

Bisection points to 8f91f38 https://reviews.llvm.org/D86142 which is odd because at first glance I wouldn't think that patch should affect non-fortran.

Should those gcov symbols have been considered mergeable?

Our build setup is pretty complex and I'm still trying to understand the failure better, but I wanted to report this as early as possible given the 12.0.0 timeline. Right now I only have the CI logs but I'll try reproducing it locally. If there are any specific pieces of information that I should try to extract to help people debug this, please let me know.

@llvmbot
Copy link
Member Author

llvmbot commented Feb 17, 2021

As luck would have it, I can't reproduce this locally.

By adding --no-fortran-common during the cargo linking, I can make the CI builds succeed.

Though, it's still not clear to me what part of the system is on the hook for this. Should it be seen as an lld bug due to breaking backwards compatibility? Should rust/cargo be the ones responsible for passing this flag for me? Should it be on me the caller to do this?

@MaskRay
Copy link
Member

MaskRay commented Feb 18, 2021

This is assuredly a rustlib/x86_64-unknown-linux-gnu/lib/libprofiler_builtins-a2dfa79aebcd4dda.rlib issue.

[task 2021-02-17T15:08:41.732Z] 15:08:41 INFO - >>> defined at GCDAProfiling.c:653 (../../src/llvm-project/compiler-rt/lib/profile/GCDAProfiling.c:653)
[task 2021-02-17T15:08:41.732Z] 15:08:41 INFO - >>> GCDAProfiling.o:(__gcov_dump) in archive /builds/worker/fetches/rustc/lib/rustlib/x86_64-unknown-linux-gnu/lib/libprofiler_builtins-a2dfa79aebcd4dda.rlib
[task 2021-02-17T15:08:41.732Z] 15:08:41 INFO - >>> defined at GCDAProfiling.c
[task 2021-02-17T15:08:41.732Z] 15:08:41 INFO - >>> GCDAProfiling.c.o:(.text.__gcov_dump+0x0) in archive /builds/worker/fetches/clang/lib/clang/12.0.0/lib/linux/libclang_rt.profile-x86_64.a

I guess libprofiler_builtins-a2dfa79aebcd4dda.rlib probably shadows some definitions in libclang_rt.profile-x86_64.a . Some had been working, until ld.lld started to follow GNU ld behavior and made this trick stop working.

Historically common symbols don't have greatly defined behaviors.

Any object file with common symbols (SHN_COMMON or (rare) STT_COMMON) may observed changed behaviors with the LLD patch.

LLD<12, gold, macOS ld64 and GNU ld before 1999-12 have the --no-fortran-common behavior.
LLD>=12, GNU ld since 1999-12 have the --fortran-common behavior.

clang 11 and gcc 10 default to -fno-common, so you probably will not observe SHN_COMMON from C code with newer compilers.

It is indeed unfortunate that LLD changed the behavior to convenience some legacy PowerPC users and I do hope one day we can forget common symbols. But the linker does not really provide promise here.

libprofiler_builtins-a2dfa79aebcd4dda.rlib likely has some common symbols. You can inspect them with readelf -Ws. Converting them from common symbols to regular definitions is the appropriate fix.

@llvmbot
Copy link
Member Author

llvmbot commented Feb 18, 2021

Thank you for the explanation.

Fascinating, it looks like libprofiler_builtins compiles these parts directly from the LLVM tree: https://searchfox.org/rust/source/library/profiler_builtins/build.rs I guess it's no surprise that there's fragility there.

@MaskRay
Copy link
Member

MaskRay commented Feb 18, 2021

Additional tip: shadowing an archive's definitions is not recommended. If you can reorganize the build by removing multiple definition parts from the two archives, please consider doing so.

If you have to shadow an archive (a.a b.a): for each member of b.a, say, b.o, make sure its definition set is all defined by a.a (usually, you want to have an object file, or an archive member, which provides a super set of b.o's definitions), and make sure the early definitions do not have common symbols.

Usually, you just want to make sure you don't have any common symbol at all, in the object files and archive files that you can control.

@llvmbot
Copy link
Member Author

llvmbot commented Feb 18, 2021

Looking through the config file for this code coverage build, I see that we have export LIBS="-lclang_rt.profile-x86_64", maybe this is unnecessary (at least for Rust) since Rust apparently provides its own profiler builtins.

@tstellar
Copy link
Collaborator

This bug was not resolved in time for the 12.0.0 release, so it will have to wait for 12.0.1.

If you feel this is a high-priority bug that should be fixed for 12.0.0, please re-add release-12.0.0 to the Blocks field and add a comment explaining why this is high-priority.

@abpostelnicu
Copy link
Collaborator

I what patch was this fixed?

@tstellar
Copy link
Collaborator

mentioned in issue #48661

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
MaskRay added a commit that referenced this issue Mar 30, 2022
D86142 introduced --fortran-common and defaulted it to true (matching GNU ld
but deviates from gold/macOS ld64). The default state was motivated by transparently
supporting some FORTRAN 77 programs (Fortran 90 deprecated common blocks).
Now I think it again. I believe we made a mistake to change the default:

* this is a weird and legacy rule, though the breakage is very small
* --fortran-common introduced complexity to parallel symbol resolution and will slow down it
* --fortran-common more likely causes issues when users mix COMMON and
  STB_GLOBAL definitions (see #48570 and
  https://maskray.me/blog/2022-02-06-all-about-common-symbols).
  I have seen several issues in our internal projects and Android.
  On the other hand, --no-fortran-common is safer since
  COMMON/STB_GLOBAL have the same semantics related to archive member extraction.

Therefore I think we should switch back, not punishing the common uage.
A platform wanting --fortran-common can implement ld.lld as a shell script
wrapper around `lld -flavor gnu --fortran-common "$@"`.

Reviewed By: ikudrin, sfertile

Differential Revision: https://reviews.llvm.org/D122450
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
D86142 introduced --fortran-common and defaulted it to true (matching GNU ld
but deviates from gold/macOS ld64). The default state was motivated by transparently
supporting some FORTRAN 77 programs (Fortran 90 deprecated common blocks).
Now I think it again. I believe we made a mistake to change the default:

* this is a weird and legacy rule, though the breakage is very small
* --fortran-common introduced complexity to parallel symbol resolution and will slow down it
* --fortran-common more likely causes issues when users mix COMMON and
  STB_GLOBAL definitions (see llvm/llvm-project#48570 and
  https://maskray.me/blog/2022-02-06-all-about-common-symbols).
  I have seen several issues in our internal projects and Android.
  On the other hand, --no-fortran-common is safer since
  COMMON/STB_GLOBAL have the same semantics related to archive member extraction.

Therefore I think we should switch back, not punishing the common uage.
A platform wanting --fortran-common can implement ld.lld as a shell script
wrapper around `lld -flavor gnu --fortran-common "$@"`.

Reviewed By: ikudrin, sfertile

Differential Revision: https://reviews.llvm.org/D122450
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla lld:ELF
Projects
None yet
Development

No branches or pull requests

4 participants