windows: support reproducible native and cross toolchains (R7, depends on #2418) - #2422
windows: support reproducible native and cross toolchains (R7, depends on #2418)#2422cpunion wants to merge 181 commits into
Conversation
Provide direct Unix and Win32 thread, TLS, synchronization, atomic, stdio, setjmp, libuv, and OS adapters. Keep target-specific libraries and layouts isolated so Windows support does not add code to existing Unix binaries.
Lower PE/COFF dynamic imports, align the UCRT setjmp ABI, terminate through runtime.exit, and keep LLGO_ROOT/module parsing portable across CRLF checkouts. Cover the compiler paths and malformed target flags.
Select the hosted thread, synchronization, and atomic backends; add Windows process, clock, syscall, GC, setjmp, and lifecycle adapters; and retain the existing Unix behavior behind target-specific files. Reuse the Go Windows syscall wrapper surface and isolate Win32 code from other targets.
Implement syscall.NewCallback with cached libffi closures, Go-compatible signature validation, closure-identity keys, and 386 calling conventions. Route both syscall and reflect callbacks through the retained foreign-thread GC lifecycle so repeated callbacks avoid collector registration churn.
Run native PE/COFF runtime, FFI, GC, callback, thread, clock, import, and compiler-path smoke tests under the shared dependency setup. Keep the empty-program import audit alongside the feature tests so unintended runtime dependencies stay visible.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
FennoAI Review — Windows toolchain support (R7)
Reviewed the R7-only range b0676768c..af46044fa (18 files, +479/-120) as the focus area, with the wider Windows-port diff as context. The R6 dependency (#2418) was reviewed separately per the request.
Overall: The new work is clean, idiomatic, and well-covered by table-driven tests. The response-file quoting (GNU vs CommandLineToArgvW rules), MSVC import-library resolution, and the explicit toolchain-selection helpers (useSystemClangForTarget, clangDriverTargetForHost, espClangDownload) are all thoughtfully implemented with accurate explanatory comments. No blocking issues found.
A few minor observations are left inline. Highlights:
- Multi-
-Lsearch order inresolveMSVCImportLibraries: aname.libin a later-Ldir suppresses rewriting alibname.dll.ain an earlier dir, which can diverge from the linker's own per-directory resolution. Fine for single-dir cases; worth confirming against real MSYS2 layouts. - Error context: the
llvm-ararchive step returnscmd.Run()'s error bare — wrapping it would ease diagnosing Windows CI failures. - Magic-string coupling between
espClangDownloadandgetESPClangPlatformvia the literal"x86_64-w64-mingw32". - Redundant PATHEXT scan per PATH entry in
executableCandidates.
Adjacent (pre-existing, not changed here, mentioned for awareness): extractZip in internal/crosscompile/fetch.go lacks the Zip-Slip path-containment guard that extractTarGz already has, and downloaded toolchains have no checksum verification. Both are outside this PR's scope but live in the same subsystem this port touches.
|
@fennoai /review |
|
FennoAI is processing your request, please wait a moment... |
There was a problem hiding this comment.
Review: R7-only range b0676768c..af46044fa (18 files, +479/-120)
Scoped to the R7-only diff as requested. Overall this is a clean, well-reasoned change: the NativeToolchain/ResponseFileStyle typed capabilities, the Windows vs GNU response-file quoting split (CommandLineToArgvW backslash-doubling), and the bsdtar-vs-MSYS-tar handling are all correct and well tested (clang_test.go, crosscompile_test.go). One inline nit below; two adjacent-context items worth a decision are noted here.
Adjacent context (not introduced by the changed lines, but touched by this PR's theme):
-
No integrity/checksum verification of the new Windows toolchain download. This diff adds a new third-party executable source
espClangWindowsBaseUrl = "https://github.com/espressif/llvm-project/releases/download/esp-19.1.2_20250312"(internal/crosscompile/crosscompile.go). The archive is extracted and itsclang++/lldbinaries are executed directly, with no pinned SHA-256. HTTPS protects transport but not artifact contents. Consider pinning and verifying a digest for each download URL. (Pre-existing gap for other sources too, but this PR introduces a new source.) -
Zip-slip guard missing in
extractZip(internal/crosscompile/fetch.go).extractTarGzrejects entries escapingdest(strings.HasPrefix(target, filepath.Clean(dest)+separator)), butextractZipjoinsfile.Nametodestwith no equivalent check.extractZipitself is outside the changed lines, but it sits beside theextractTarXzrefactor in this diff and is reachable for.ziplib downloads. Worth mirroring the tar guard (and a traversal test case) since the extraction paths are being reworked here.
|
FennoAI is processing your request, please wait a moment... |
There was a problem hiding this comment.
Review: full PR diff vs main (6fe8babd...af46044f)
This /review had no focus, so this pass covers the full PR diff against main. Note the PR is the stacked R6 (#2418) + R7 change, so its diff vs main is large (~394 files) and includes the R6 runtime/stdlib work that the author states was reviewed separately. I concentrated the four review passes on the substantive non-fixture source (runtime Windows code, internal/build, internal/clang, internal/crosscompile, xtool/env, cl/compile.go) and skipped the mechanical _testgo/_testrt fixture regenerations.
Overall this is high-quality, unusually well-documented work: the Windows runtime shims (SEH fault handling, FLS/TLS lifecycle, WaitOnAddress sema, BCryptGenRandom), the COFF/MSVC toolchain handling, and the response-file quoting split all held up under review, with strong compile-time ABI assertions. Documentation-accuracy found no issues. One inline nit below; a few items already raised in the prior R7-scoped review are summarized here for completeness.
Already noted in the prior R7 review (not re-posting inline):
- Zip-slip guard missing in
extractZip(internal/crosscompile/fetch.go):extractTarGzrejects entries escapingdest, butextractZipdoes not, though it's reachable via.ziplib downloads (e.g. picolibc). Pre-existing, but worth mirroring the tar guard. - No checksum/signature verification of downloaded toolchain archives that are later extracted and executed (
fetch.go/crosscompile.go), including the new Windows ESP Clang source. Consider pinning SHA-256 per URL. xtool/env/env.go: innerpathloop variable shadows the outer PATH string, andPATHEXTis re-scanned/allocated once per PATH entry despite being invariant.
1 participant
Depends on #2418.
Part of #2325.
Summary
PATHEXTDependency
This R7 change is stacked on R6 so the toolchain paths are exercised with the Windows runtime and standard-library support from #2418. The R7-only diff is 19 files with 536 additions and 123 deletions.
Validation
c96adb17ahas SHA-256b66b05e67ccf084073829486df44a1debc194f3e2584b8e3a4270b747fad8580GOMAXPROCS=2 go test -count=1 -p=2 -timeout=20m ./internal/clang ./internal/crosscompile/... ./xtool/env/...GOMAXPROCS=2 go test -count=1 -p=2 -run '^$' -timeout=10m ./internal/clite/ffifrom the runtime modulegit diff --checkThe contribution head reruns the full CI matrix after each dependency update.