-
Notifications
You must be signed in to change notification settings - Fork 18k
cmd/go: TestScript/cgo_stale_precompiled fails with Clang 14.0.6 #64423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
CC @golang/windows, @bcmills. |
I suspect that this is a bug in We invoke it to compute the cache ID, but if it fails we currently omit that ID from the cache inputs instead of erroring out or caching the error text: That seems like a bug. |
I'm not able to reproduce. Maybe the output of My gcc output
|
Just after posting that, I realized I forgot to configure
|
Yep, it's not expecting the |
I think https://github.com/llvm/llvm-project/blob/3a6f02a6581b49b269710eea944dc114166403ed/clang/lib/Basic/Version.cpp#L95-L97 means that any build of clang that sets An official clang is just On Mac, https://stackoverflow.com/a/70697937 says its clang (Xcode's build of clang?) may give you I haven't traced the code down to know for certain that this is the code path being used, it just seems reasonable. I assume the buildid.go loop only checks fields 1 and 2 to avoid false positives. I'm not sure what a good fix would be without risk. Keep track of a loose match in the loop so if no perfect match is found, it falls back to the first line with a |
It appears to be passing on the Go project's macOS builders — I see a passing run in https://results.usercontent.cr.dev/invocations/build-8763160122012974529/tests/cmd%2Fgo.TestScript%2Fcgo_stale_precompiled/results/bc3b68df-24354/artifacts/output?token=AXsiX2kiOiIxNzAxMjA3OTY1NjgwIiwiX3giOiIzNjAwMDAwIn2mPsMH_r8cNPO3GylSLoU3W2t2sCM_9iRp8sl88xW4jQ, for example. I suspect that's because |
Ah, yep, I got mixed up, it makes sense that it works in those cases. Also didn't realize those builders used clang!
Maybe, but I've also seen a varying number of spaces from
However, could chain these together: look for I noticed that you can already force a false positive with the current logic by putting the compiler in a directory with
The test passes, but the logic seems like it must be picking up the InstalledDir line. The full compiler path is a decent amount of information, so maybe it's ok for this purpose anyway. I'm wondering now if there's something different that can be done here, like run the clang preprocessor to unambiguously grab |
Ah, of course we could get the full string if we get |
Huh. I can also reproduce this failure with Clang on Linux:
|
Now I'm wondering why it's not failing on the |
That's why: its
|
Shouldn't this have worked because Either way, I guess it indicates a better general solution is needed. |
Marking as release-blocker because this test failure indicates a substantial compatibility problem with modern versions of a widely-used C toolchain. |
I think the full path in
|
Ah, that one is a test bug: we were forgetting to remove an explicit |
I have a fix, but it's hard to write a regression test because of other bugs (#64580). 😞 |
Change https://go.dev/cl/547998 mentions this issue: |
Change https://go.dev/cl/547997 mentions this issue: |
…precompiled Otherwise, if make.bash produced a relative default CC path but the user has an absolute path to CC set in their environment, the test will fail spuriously. For #64423. Change-Id: I0f3e1d04851585e1b39266badcda9f17489332d9 Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest Reviewed-on: https://go-review.googlesource.com/c/go/+/547997 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Matloob <[email protected]> Auto-Submit: Bryan Mills <[email protected]>
Change https://go.dev/cl/548120 mentions this issue: |
In CL 547998 I relaxed cmd/go's parsing of version lines to allow it to recognize clang versions with vendor prefixes. To prevent false-positives, I added a check for a version 3-tuple following the word "version". However, it appears that some releases of GCC use only a 2-tuple instead. Updates #64423. Fixes #64619. Change-Id: I5f1d0881b6295544a46ab958c6ad4c2155cf51fe Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest Reviewed-on: https://go-review.googlesource.com/c/go/+/548120 TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Michael Matloob <[email protected]> Run-TryBot: Bryan Mills <[email protected]> Auto-Submit: Bryan Mills <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
…precompiled Otherwise, if make.bash produced a relative default CC path but the user has an absolute path to CC set in their environment, the test will fail spuriously. For golang#64423. Change-Id: I0f3e1d04851585e1b39266badcda9f17489332d9 Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest Reviewed-on: https://go-review.googlesource.com/c/go/+/547997 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Matloob <[email protected]> Auto-Submit: Bryan Mills <[email protected]>
To better diagnose bugs like this one in the future, I think we should also refuse to use a C compiler if we can't identify a sensible version for it. I did not do that in this CL because I want it to be small and low-risk for possible backporting. Fixes golang#64423. Change-Id: I21e44fc55f6fcf76633e4fecf6400c226a742351 Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest Reviewed-on: https://go-review.googlesource.com/c/go/+/547998 Auto-Submit: Bryan Mills <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Michael Matloob <[email protected]>
In CL 547998 I relaxed cmd/go's parsing of version lines to allow it to recognize clang versions with vendor prefixes. To prevent false-positives, I added a check for a version 3-tuple following the word "version". However, it appears that some releases of GCC use only a 2-tuple instead. Updates golang#64423. Fixes golang#64619. Change-Id: I5f1d0881b6295544a46ab958c6ad4c2155cf51fe Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest Reviewed-on: https://go-review.googlesource.com/c/go/+/548120 TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Michael Matloob <[email protected]> Run-TryBot: Bryan Mills <[email protected]> Auto-Submit: Bryan Mills <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
yes.
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
clang get from https://winlibs.com/
GCC 12.1.0 + LLVM/Clang/LLD/LLDB 14.0.6 + MinGW-w64 10.0.0 (MSVCRT) - release 3
In cmd
cd $GOROOT/src
go test -v cmd/go -run=TestScript/cgo_stale_precompiled
What did you expect to see?
test pass.
What did you see instead?
Output
The text was updated successfully, but these errors were encountered: