build: prepare the LLVM 21 upgrade - #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 |
cb421a2 to
4b6e55f
Compare
4b6e55f to
bf6d6ef
Compare
1 participant
Summary
Os/Ozto the numeric optimization level accepted by LLD's LTO optionThis PR is the compatibility and payload-contract layer only. It intentionally does not switch the default LLVM major or reference an unpublished LLVM 21 payload.
Validation
go testforinternal/llvmpayload,internal/crosscompile,internal/build, andssago test -tags=llvm19forinternal/crosscompile,internal/build, andssaltopluginagainst LLVM 19.1.7 and LLVM 21.1.8git diff --checkThe branch is rebased on current
main(4a5bd7615). A separate follow-up will switch the default only after the LLVM binding and all four payload assets have tagged releases.