build: upgrade LLGo to LLVM 21 - #2334
Open
zhouguangyuan0718 wants to merge 28 commits into
Open
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: LLVM 21 upgrade
Solid, well-tested change. The new internal/llvmpayload package cleanly centralizes the previously-scattered ESP Clang constants, SHA256 verification (Go and bash, verified before extraction, with cleanup on mismatch and a dedicated negative test) is a real integrity improvement, and the bash hardening (set -euo pipefail, trap cleanup, download-then-verify-then-extract) is correct and consistent with the Go path.
A few items are worth addressing before merge — most importantly whether the LLVM 21 payload/toolchain path is actually functional, and the platform-specific linker/compiler names in the new toolchain validator. See inline comments.
Findings that have no reliable inline location:
- Docs still pin LLVM 19.
ltoplugin/README.md(build instructions,-DLLVM_DIR=/path/to/llvm-19/..., and the "LLVM 19ld64.lld" wording) still hard-codes LLVM 19 even thoughCMakeLists.txtnow accepts 19.x or 21.x andinternal/lto/lto.gowas de-versioned to "the bundled ld64.lld". Similarly,README.md(generated fromdoc/_readme/scripts/install_macos.sh/install_ubuntu.sh) still installs onlyllvm@19/llvm-19and does not mention the new toolchain-major validation requirement introduced byvalidateLLVMToolchain— a user who builds with-tags llvm21but installs LLVM 19 tools will now hit "LLVM major version mismatch". Consider updating these docs (fix belongs in thedoc/_readme/scripts/*.shsources for the generated README). - WASI SDK download remains unverified (pre-existing, low). Now that the checksum plumbing exists (
downloadAndExtractArchiveWithChecksum), the WASI SDK download still passes an empty checksum and skips verification. Not introduced here, but a natural candidate to pin next since this PR is about hardening this path.
Comment on lines
+33
to
+44
| var manifests = map[int]manifest{ | ||
| 19: { | ||
| llvmMajor: 19, | ||
| version: "19.1.2_20250905-3", | ||
| sha256: map[string]string{ | ||
| "aarch64-apple-darwin": "4f15d18c93eabdace3eab901582e528ac334d328fb8f19f153ee55b2208d101b", | ||
| "aarch64-linux-gnu": "b2d8e77bbf3394c6a1f0d66e59385d78d2b49b97ebe782e612cba7f93dcb2337", | ||
| "x86_64-apple-darwin": "e4f329a911e813ee825984f039578614dc0fe69001c2afe3e61edf27821be3ad", | ||
| "x86_64-linux-gnu": "e2e0c48cd76e45ceba910917a2a97988dc80e3bb6040ea262bfe9293d5d9ac57", | ||
| }, | ||
| }, | ||
| } |
Contributor
There was a problem hiding this comment.
[P1] No LLVM 21 payload: ESP cross-compile fails under -tags llvm21
This PR adds llvm21 build-tag config files and widens the LTO plugin to accept LLVM 21, but manifests contains only the 19 entry and DefaultMajor is still 19. When LLGo is built with -tags llvm21, gllvm.Version reports 21.x, so getESPClangRoot -> llvmpayload.ForLLVMVersion("21...") -> ForMajor(21) returns "no LLGo LLVM payload for major version 21" (crosscompile.go:141-144), and any ESP/cross-compile build on an LLVM 21 toolchain fails.
If the LLVM 21 ESP payload simply is not published yet and 19-only downloads are intentional for now, that is fine — but it should be documented, and the package doc comment ("toolchains distributed with and downloaded by LLGo", ForLLVMVersion example 21.1.8) overstates availability. As written, the headline "upgrade to LLVM 21" is not functional for the ESP download path.
Comment on lines
+415
to
+417
| if err := validateLLVMToolchain(export); err != nil { | ||
| return nil, fmt.Errorf("invalid LLVM toolchain: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
[P2] validateLLVMToolchain runs 3 uncached --version subprocesses on every Build
Build now unconditionally calls validateLLVMToolchain, which spawns three subprocesses (llvm-config, clang/export.CC, ld.lld) via exec.Command(tool, "--version"). For a one-shot llgo build this is negligible. But in-process, high-fan-out drivers (the cltest harness, go test suites that call Build/Do once per package) will spawn hundreds x 3 --version processes per run, all producing the same answer since gllvm.Version and the toolchain paths are constant for the process lifetime. Consider memoizing the result per (linkedVersion, toolPath) in ValidateToolchainMajor so repeated Build calls pay the cost once.
Comment on lines
+108
to
+121
| func PlatformSuffix(goos, goarch string) (string, bool) { | ||
| switch goos + "/" + goarch { | ||
| case "darwin/amd64": | ||
| return "x86_64-apple-darwin", true | ||
| case "darwin/arm64": | ||
| return "aarch64-apple-darwin", true | ||
| case "linux/amd64": | ||
| return "x86_64-linux-gnu", true | ||
| case "linux/arm64": | ||
| return "aarch64-linux-gnu", true | ||
| default: | ||
| return "", false | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
[P2] PlatformSuffix drops linux/arm and windows/amd64 mappings
The refactored getESPClangPlatform now delegates to PlatformSuffix, which only maps darwin/linux amd64/arm64. The previous inline switch also handled linux/arm (arm-linux-gnueabihf) and windows/amd64 (x86_64-w64-mingw32). If any target config or CI relied on those, the download path now returns "" -> "platform ... is not supported for download". If the removal is intentional (those payloads never shipped), please confirm; otherwise it is a regression.
Comment on lines
+32
to
+33
| fmt.Printf("LLGO_LLVM_MAJOR=%s\n", fmt.Sprint(manifest.LLVMMajor())) | ||
| fmt.Printf("ESP_CLANG_LLVM_MAJOR=%s\n", fmt.Sprint(manifest.LLVMMajor())) |
Contributor
There was a problem hiding this comment.
[P3] Unnecessary fmt.Sprint on int; use %d
LLVMMajor() returns int; fmt.Printf("...=%s\n", fmt.Sprint(manifest.LLVMMajor())) formats an int to string then re-formats as %s. Use %d directly. Also note the mixed error handling below: a missing platform mapping panics while a missing artifact does os.Exit(1). For a CLI feeding $GITHUB_ENV, prefer the stderr + os.Exit(1) form for both so CI failures are readable without a Go stack trace.
Comment on lines
+805
to
+811
| func validateLLVMToolchain(export crosscompile.Export) error { | ||
| if export.ClangRoot != "" { | ||
| binDir := filepath.Join(export.ClangRoot, "bin") | ||
| return envllvm.ValidateToolchainMajor(gllvm.Version, | ||
| filepath.Join(binDir, "llvm-config"), | ||
| filepath.Join(binDir, "clang"), | ||
| filepath.Join(binDir, "ld.lld"), |
Contributor
There was a problem hiding this comment.
[P1] Toolchain validator hardcodes ld.lld/clang (breaks darwin/windows)
validateLLVMToolchain always probes ld.lld, but the linker is platform-specific: darwin toolchains ship ld64.lld and windows uses lld-link (crosscompile.go:290,295). ClangRoot is set to the ESP Clang bundle for darwin and linux hosts, so on macOS filepath.Join(binDir, "ld.lld") does not exist and ValidateToolchainMajor fails with query LLVM tool ... ld.lld ... no such file, aborting the build on exactly the darwin release path this PR cares about.
Also, the ClangRoot branch probes filepath.Join(binDir, "clang"), but the bundle and export.CC only guarantee clang++ (crosscompile.go:217). Prefer validating export.CC and deriving the linker name from the target GOOS (mirroring the crosscompile linker logic) instead of hardcoding ld.lld/clang.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
zhouguangyuan0718
force-pushed
the
codex/llvm21-upgrade
branch
2 times, most recently
from
August 15, 2026 08:42
4b6e55f to
bf6d6ef
Compare
zhouguangyuan0718
force-pushed
the
codex/llvm21-upgrade
branch
from
August 27, 2026 13:29
152cb88 to
d243601
Compare
zhouguangyuan0718
changed the title
zhouguangyuan0718
force-pushed
the
codex/llvm21-upgrade
branch
4 times, most recently
from
August 28, 2026 09:47
0e3c4ae to
3ec7475
Compare
This was referenced Aug 28, 2026
zhouguangyuan0718
force-pushed
the
codex/llvm21-upgrade
branch
from
August 29, 2026 11:38
ae8a18d to
c4c910c
Compare
zhouguangyuan0718
force-pushed
the
codex/llvm21-upgrade
branch
from
September 1, 2026 02:31
c4c910c to
7f7b5ad
Compare
zhouguangyuan0718
force-pushed
the
codex/llvm21-upgrade
branch
from
September 1, 2026 05:17
abe561e to
2889aba
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
21.1.3_20260816for release archives and automatic cross-toolchain downloads, with checked-in SHA-256 verificationgoplus/compiler-rttagxtensa_release_21.1.3_20260408for cross-runtime buildsLLGOLTOPluginTemporary Go LLVM dependency
Until xgo-dev/llvm#48 is merged and tagged, this branch temporarily replaces
github.com/xgo-dev/llvmwith the exact fork pseudo-version:github.com/zhouguangyuan0718/go-llvm v0.0.0-20260827144345-45af6df6739aThat version resolves to PR head
45af6df6739a8be0168070ac35ab63170f1378e3, including theLLVMIsOpaqueStructbinding needed to preserve defined empty structs across LLVM contexts. After the binding release is available, the temporaryreplacewill be removed and the normalgithub.com/xgo-dev/llvmrequirement will be updated before merge.Validation
main(ff9bad850)go testpassed forinternal/dcepass,internal/llvmpayload, allinternal/crosscompile/...packages,internal/build,internal/littest,ssa, andxtool/env/llvm, using the remotely downloadable fork pseudo-versionLLGOLTOPlugin.dylibagainst LLVM 21.1.8 with ccache21.1.3_20260816, and all four published artifact digestsgit diff --check2e9d23ff6, the release build and Linux amd64/arm64 artifact smoke tests pass with complete GNU triples and no GCC-specific workaround, while the remaining matrix finishes