windows: support independent MSVC and MinGW profiles (R9, depends on goplus/lib#26) - #2440
Conversation
There was a problem hiding this comment.
Review: Windows native (MinGW/GNU) toolchain support
Solid, well-structured change. The NativeToolchain abstraction is clean, toolchain-identity fields are consistently threaded through the fingerprint (empty() + collectCommonInputs + TestNativeToolchainIdentityParticipatesInFingerprint), and the tricky bits are handled correctly:
- GNU vs Windows response-file quoting (
clang.gowriteGNUResponseArg/writeWindowsResponseArg) implements theCommandLineToArgvWbackslash-doubling rule rather than escaping naively. usesWindowsCABI(cabi.go) correctly reclassifies MinGW as native Windows C ABI while excluding MSYS/Cygwin, with matching table-driven tests.- CI archive handling in
setup-deps/action.ymlis properly hardened (SHA-256 pinned LLVM/FileCheck, pinned vcpkg baseline,--fail/--retry, HTTPS,windows-abiallowlisted tomsvc|mingw).
No blocking correctness bugs found. A few maintainability/robustness items are inline. One out-of-scope note:
- Pre-existing zip-slip in
internal/crosscompile/fetch.goextractZip(~line 400): unlikeextractTarGz(line 280),extractZipjoinsdest+file.Nameand writes with no path-containment check, so a crafted.zipentry (../..) can escapedest. Not touched by this PR and the download URLs are HTTPS constants, but the native-Windows work continues to rely on this shared extractor. Worth adding the samestrings.HasPrefix(target, filepath.Clean(dest)+sep)guard thatextractTarGzalready has.
Findings without inline locations
internal/crosscompile/crosscompile.go:461:getESPClangRoot(forceEspClang)runs unconditionally here and can perform a download or return an error. For the native-Windows path below (goos == "windows"), the entireexportstruct is reassigned wholesale byresolveWindowsToolchainandclangRoot/export.ClangRootare then cleared. That means the ESP-clang resolution above is wasted work for native Windows and, worse, a spurious failure path: a download hiccup ingetESPClangRootcan fail a build that never uses ESP clang. Consider short-circuiting the ESP-clang resolution when the native Windows toolchain will be selected.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
|
已按评审修正:d813a2503 在 native Windows profile 解析前不再触发无关的 ESP Clang 查找或下载,同时补充了分支判定测试; |
d813a25 to
0b66e0c
Compare
Part of xgo-dev/llgo#2325.
R8 (xgo-dev/llgo#2425) and the native Windows calling-convention support (xgo-dev/llgo#2433) are merged. This R9 contribution still depends on the minimal Windows C bindings in goplus/lib#26; only the five demo/test/tool modules that consume those bindings use the temporary fork revision, while LLGo's root compiler module no longer depends on
goplus/lib.Summary
windows/amd64MSVC and GNU/MinGW profiles without changingGOOS,GOARCH, Go flags, or LLGo's existing-targetmeaning;CCis unset, probe the effective target of the defaultclang: official standalone LLVM selectsx86_64-pc-windows-msvc, while MSYS2 CLANG64 selectsx86_64-w64-windows-gnu;CC,CXX, and-ldflags=-extld=...overrides Go-compatible, including quoted compiler commands and empty external-linker flag values;x64-windowsdependencies without resolving compiler libraries from MSYS2;exe,c-archive, andc-sharedartifacts interoperable within each profile, including explicit MinGW COFF import-library generation;msys-2.0.dll,cygwin1.dll, orlibwinpthread, and the MinGW lane cannot silently reuse MSVC SDK/vcpkg state.User-facing selection
No LLGo-specific profile variable is required:
LLGo probes the default Clang target and appends the matching canonical target internally. Explicit
CC,CXX, and external-linker settings remain optional Go-compatible overrides. CI verifies installedllgo.exeagain withCC,CXX,PKG_CONFIG, andPKG_CONFIG_PATHunset from the profile's normal shell.Dependency boundary
The temporary
github.com/cpunion/libreplacement is limited to_demo/c,_xtool,cl/_testlibc,cl/_testpy, andcl/_testrt. It will be replaced with the releasedgithub.com/goplus/libversion after goplus/lib#26 merges. The root module, compiler, SSA, C ABI, runtime, and build system do not import that fork.Validation
llgo install ./...passes;CC.Deliberate boundaries
clang-clsyntax and non-Clang drivers fail clearly until a complete driver-dialect implementation exists.