Skip to content

[lli] Fix crash with --no-process-syms on MinGW #151386

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

Merged
merged 4 commits into from
Aug 2, 2025

Conversation

jeremyd2019
Copy link
Contributor

@jeremyd2019 jeremyd2019 commented Jul 30, 2025

In this case, J->getProcessSymbolsJITDylib() returns a NULL pointer. In order to make sure __main is still defined, add the symbol to J->getMainJITDylib() instead in that case. This returns a reference and thus cannot be NULL.

Fixes #143080

In this case, J->getProcessSymbolsJITDylib() returns a NULL pointer.  In
order to make sure __main is still defined, add the symbol to
J->getMainJITDylib() instead.  This returns a reference and thus cannot
be NULL.

Fixes llvm#143080
@jeremyd2019 jeremyd2019 requested a review from lhames July 30, 2025 20:13
@jeremyd2019
Copy link
Contributor Author

jeremyd2019 commented Jul 30, 2025

With this change, llvm/test/ExecutionEngine/OrcLazy/emulated-tls.ll succeeds on Cygwin (it probably would on MinGW also), where before it crashed. I was originally just going to add if (J->getTargetTriple().isOSCygMing() && J->getProcessSymbolsJITDylib()) but that then caused the emulated-tls.ll test to fail due to __main also being undefined.

@lhames
Copy link
Contributor

lhames commented Jul 31, 2025

The process-symbols dylib is the right place for __main to go usually, it's only when we pass -no-process-syms that we'd want to put it anywhere else. (Side note: If / when we make Platforms mandatory we should put it in the Platform dylib instead).

Would you mind changing this to:

auto &WorkaroundJD = J->getProcessSymbolsJITDylib()
    ? *J->getProcessSymbolsJITDylib()
    : J->getMainJITDylib();
ExitOnErr(WorkaroundJD.define(
        orc::absoluteSymbols({{J->mangleAndIntern("__main"),
                               {orc::ExecutorAddr::fromPtr(mingw_noop_main),
                                JITSymbolFlags::Exported}}})));

@jeremyd2019
Copy link
Contributor Author

I had done something like that in a previous attempt, but your suggestion is cleaner than what I was trying with a pointer.

Copy link
Contributor

@lhames lhames left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @jeremyd2019!

@jeremyd2019 jeremyd2019 merged commit 65990d6 into llvm:main Aug 2, 2025
9 checks passed
@jeremyd2019 jeremyd2019 deleted the lli-crash-fix branch August 2, 2025 01:16
@llvm-ci
Copy link
Collaborator

llvm-ci commented Aug 2, 2025

LLVM Buildbot has detected a new failure on builder clang-hip-vega20 running on hip-vega20-0 while building llvm at step 2 "update-annotated-scripts".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/24552

Here is the relevant piece of the build log for the reference
Step 2 (update-annotated-scripts) failure: update (failure)
git version 2.34.1
fatal: unable to access 'https://github.com/llvm/llvm-zorg.git/': Could not resolve host: github.com
fatal: unable to access 'https://github.com/llvm/llvm-zorg.git/': Could not resolve host: github.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Cygwin] lli crashes with --no-process-syms
3 participants