build: upgrade LLGo to LLVM 21 - #2334
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
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.
| 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", | ||
| }, | ||
| }, | ||
| } |
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.
| if err := validateLLVMToolchain(export); err != nil { | ||
| return nil, fmt.Errorf("invalid LLVM toolchain: %w", err) | ||
| } |
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.
| 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 | ||
| } | ||
| } |
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.
| fmt.Printf("LLGO_LLVM_MAJOR=%s\n", fmt.Sprint(manifest.LLVMMajor())) | ||
| fmt.Printf("ESP_CLANG_LLVM_MAJOR=%s\n", fmt.Sprint(manifest.LLVMMajor())) |
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.
| 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"), |
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
Compared with |
4b6e55f to
bf6d6ef
Compare
152cb88 to
d243601
Compare
zhouguangyuan0718
changed the title
0e3c4ae to
3ec7475
Compare
ae8a18d to
c4c910c
Compare
1 participant
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