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

Remove support for older LLVM versions, and re-order linker flags #105110

Merged
merged 53 commits into from
Aug 1, 2024

Conversation

directhex
Copy link
Contributor

We have generally tried to support linking multiple versions of LLVM within our git tree. Every new LLVM version moves symbols around between libraries, and as a result, every new version of LLVM requires different linker flags to build. The command line tool llvm-config should tell you the exact flags you need, but it is a problem for us when cross-compiling to rely on this, and as a result, we transcribe the result of various llvm-config outputs directly into Mono's CMakeLists.txt.

In an effort to support multiple versions of LLVM, flags common between all supported versions were kept in one place, then the version-specific flags appended afterwards. And this has worked fine for years. However:

  1. Whilst we only link with lld, it is common for contributors and source-build to link with gold, bfd, or some other GNU-flavoured linker, where library order is essential
  2. The list of common libraries to link has remained unchanged for years, but the symbol intra-dependencies may have changed a long time ago, so common symbol order cannot be assumed to remain valid between LLVM versions

This has resulted in a long-standing problem for people using e.g. Debian or Ubuntu or GitHub CodeSpaces, rather than always building with one of our dockerfiles representing our "real" build environment, when targeting platforms which use Mono and link LLVM.

We have generally tried to support linking multiple versions of LLVM
within our git tree. Every new LLVM version moves symbols around between
libraries, and as a result, every new version of LLVM requires different
linker flags to build. The command line tool `llvm-config` should tell
you the exact flags you need, but it is a problem for us when
cross-compiling to rely on this, and as a result, we transcribe
the result of various llvm-config outputs directly into Mono's
CMakeLists.txt.

In an effort to support multiple versions of LLVM, flags common between
all supported versions were kept in one place, then the version-specific
flags appended afterwards. And this has worked fine for years. However:

1. Whilst we only link with `lld`, it is common for contributors and
   source-build to link with `gold`, `bfd`, or some other GNU-flavoured
   linker, where library order is essential
2. The list of common libraries to link has remained unchanged for
   years, but the symbol intra-dependencies may have changed a long time
   ago, so common symbol order cannot be assumed to remain valid between
   LLVM versions

This has resulted in a long-standing problem for people using e.g.
Debian or Ubuntu or GitHub CodeSpaces, rather than always building
with one of our dockerfiles representing our "real" build environment,
when targeting platforms which use Mono and link LLVM.
@directhex
Copy link
Contributor Author

It looks like the linker order between LLVMIRReader and LLVMObject got codified in d2939d3 3 years ago. I'd need to find ancient artifacts to confirm, but my suspicion is, with LLVM 11, LLVMObject did not have a hard dependency on LLVMIRReader and thus it was fine to have the linker order put LLVMIRReader before LLVMObject (GNU linkers need the symbol consumer listed before the symbol provider). And since those libraries still existed in LLVM 12+, and the old linker order still worked (as we build with LLVM lld, which cares less about library order), we didn't notice that maintaining the old linker order forever is actually harmful.

We need to regenerate the full linker list in src/mono/CMakeLists.txt using the relevant llvm-config for every major LLVM update.

@carlossanlop
Copy link
Member

Let's run the other azp runs that are most likely affected. Just for peace of mind.

@carlossanlop
Copy link
Member

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@kg kg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested building with this PR and it appears to resolve the issues I was having on Debian Bookworm with its system packages (llvm/clang 14)

@ilonatommy
Copy link
Member

Closing and opening to re-trigger the CI after the outage

jkotas and others added 6 commits July 26, 2024 14:32
* Use ConcurrentDictionary in runtimecounters test

Fixes #105443

* Fix build break
…#105530)

* Fix issue 98506 - Excessive exceptions generated in StackTraceSymbols

* Code review feedback
There was a problem with using heap from the related LoaderAllocator for
shuffle thunk cache heap. I have tested it again and it seems that the
issue is gone.

So I am removing the workaround, making the cache use LoaderAllocator
local heap.

Close #55697
@directhex
Copy link
Contributor Author

Okay the commit history is a mess, but I updated the linkages to LLVM 19. Ignore the branch name.

@ilonatommy ilonatommy merged commit c59543c into main Aug 1, 2024
70 checks passed
@lewing lewing deleted the make-llvm-16-required-and-fix-linker-order branch August 1, 2024 14:41
akoeplinger added a commit to akoeplinger/runtime that referenced this pull request Aug 2, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.