Add LLVM 21 support while keeping LLVM 19 as default#35
Add LLVM 21 support while keeping LLVM 19 as default#35xushiwei merged 7 commits intoxgo-dev:goplusfrom
Conversation
This matches TinyGo, where they are merged. It should make maintenance a little bit easier, and also makes it easier to add new OSes (like FreeBSD) without having to create a whole new set of files.
add ExecutionEngine.GetFunctionAddress()
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the project to support LLVM 21, integrating recent upstream changes while maintaining LLVM 19 as the default version. It streamlines the LLVM configuration by consolidating platform-specific files into unified, version-based configurations and introduces new JIT compilation and function address retrieval functionalities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for LLVM 21 and refactors the LLVM build configuration files, which is a great improvement for maintainability. However, I've identified a critical issue in executionengine.go. A new function, NewJITCompiler, is introduced using a C function that has been removed in recent LLVM versions (including the targeted LLVM 19 and 21), which will cause build failures. I have provided a detailed comment and a code suggestion to resolve this.
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| return | ||
| } | ||
|
|
||
| func NewJITCompiler(m Module, optLevel int) (ee ExecutionEngine, err error) { |
There was a problem hiding this comment.
ee.C = nil is redundant here — Go zero-initializes the named return value, so ee.C is already nil on entry and LLVMCreateJITCompilerForModule does not populate it on failure. The existing NewInterpreter omits this line for the same reason. Keeping it introduces a misleading asymmetry between the two constructors.
| @@ -110,6 +112,17 @@ func NewInterpreter(m Module) (ee ExecutionEngine, err error) { | |||
| return | |||
| } | |||
There was a problem hiding this comment.
NewJITCompiler wraps LLVMCreateJITCompilerForModule, which is the legacy pre-MCJIT interface and has been deprecated by LLVM in favour of MCJIT (NewMCJITCompiler already exists in this package) and newer ORC/LLJIT APIs. A doc comment is needed to:
- Signal that this is the legacy JIT.
- Direct callers toward
NewMCJITCompileror ORC as the modern alternatives.
Without this, callers cannot distinguish between the two JIT constructors in the package.
| @@ -159,6 +172,12 @@ func (ee ExecutionEngine) FindFunction(name string) (f Value) { | |||
| return | |||
There was a problem hiding this comment.
LLVMGetFunctionAddress returns 0 when the function is not found (or on failure). The binding surfaces this as a uint64(0) with no accompanying error, making it impossible for callers to distinguish "function not found" from "function is at address 0". Callers who blindly cast the result to a function pointer and call it will dereference a null pointer.
Consider mirroring the pattern of FindFunction — either return (uint64, bool) or return 0 with documentation that callers must check for zero before use. At minimum, a doc comment is needed.
Additionally, per LLVM docs, getFunctionAddress may trigger JIT compilation rather than being a simple lookup. Callers who call this in a hot path for the same name should cache the result.
| @@ -0,0 +1,19 @@ | |||
| //go:build !byollvm && !llvm14 && !llvm15 && !llvm16 && !llvm17 && !llvm18 && !llvm20 && !llvm21 | |||
There was a problem hiding this comment.
The build constraint is missing !llvm19. As written, this file activates both when no version tag is provided (acting as the default) and when -tags llvm19 is explicitly passed — which is likely intentional, but is not how any other versioned config is structured.
The risk: a future maintainer adding LLVM 22 who forgets to add !llvm22 here will silently get LLVM 19 paths in an LLVM 22 build. A short comment explaining the intentional omission and the "default fallback" pattern would prevent this.
| @@ -24,7 +24,7 @@ jobs: | |||
| # run: brew update | |||
| # Optional step when a LLVM version is very new. | |||
There was a problem hiding this comment.
The brew update guard was moved from llvm == 20 to llvm == 21, but not extended to cover 20. If the LLVM 20 Homebrew formula drifts or becomes unavailable without a brew update, macOS CI for LLVM 20 will silently fail or use a stale index. The original intent was to run brew update for the newest/freshest version. Now that 21 is the newest, consider whether 20 also still needs it, or document why only 21 requires this step.
|
The consolidation of per-OS config files and LLVM 21 CI support are clean improvements. Two broader issues worth addressing: Breaking API removals — README — |
3 participants
Summary
This PR brings in the recent upstream TinyGo changes needed for LLVM 21 support, while keeping LLVM 19 as the default version in this branch.
Included changes
ExecutionEngine.NewJITCompilerandExecutionEngine.GetFunctionAddressNotes
llvm21build tagFixes #32