Skip to content
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

cmd/link: misc/cgo/testshared failure on boringcrypto branches #49965

Open
mknyszek opened this issue Dec 3, 2021 · 7 comments
Open

cmd/link: misc/cgo/testshared failure on boringcrypto branches #49965

mknyszek opened this issue Dec 3, 2021 · 7 comments

Comments

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Dec 3, 2021

I released the 1.17.4 and 1.16.11 to the boring crypto versions of Go, and I noticed a link failure in the trybots:

https://storage.googleapis.com/go-build-log/f967062a/linux-amd64_0dc21008.log

@heschi noticed that it appeared first in https://go.dev/cl/362654 but it looked unrelated so it was ignored. We suspect this is a real failure of a flaky (EDIT: I was wrong, it's deterministic) test that was recently exposed. Low priority because it doesn't actually block boring crypto releases, but it does add noise to the process.

CC @cherrymui @randall77

@mknyszek mknyszek added this to the Backlog milestone Dec 3, 2021
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Dec 3, 2021

Do you mean that failure is nondeterministic? That would be weird as it seems from the C linker.

@heschi
Copy link
Contributor

@heschi heschi commented Dec 3, 2021

Yeah, I don't think it's flaky, just that it was hidden by the clobber test before.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Dec 3, 2021

Is it failing now? I don't see a failure in the dashboard or @mknyszek 's recent CLs.

@heschi
Copy link
Contributor

@heschi heschi commented Dec 3, 2021

See https://build.golang.org/?repo=&branch=dev.boringcrypto (the dev.boringcrypto branch). The most recent row has the failure for me.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Dec 3, 2021

Thanks. I was looking at boringcrypto.go.1.17 branch...

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Dec 3, 2021

The boringcrypto syso references pthread functions, so we need to pass -pthread to the C linker.

I spent a while looking into why this is not failing before. It turns out that on 1.17 the tests there simply don't depend on any crypto packages, so the syso is not linked in. On tip, it does depend on crypto, possibly though fuzzing as a test dependent. I can reproduce the error with 1.17 branch with a test explicitly using crypto, e.g. go test -linkshared crypto/sha1. Will send a CL soon.

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 3, 2021

Change https://golang.org/cl/369161 mentions this issue: [dev.boringcrypto] crypto/internal/boring: add -pthread linker flag

gopherbot pushed a commit that referenced this issue Dec 6, 2021
goboringcrypto_linux_amd64.syso references pthread functions, so
we need to pass -pthread to the C linker when external linking.
Usually it is automatically added when linking with runtime/cgo
package. But in shared linkage the runtime/cgo package may be in
a separate DSO and not part of this invocation.

Fixes #49965.

Change-Id: I3a9983e715ee804594a14006f212f76769ad71db
Reviewed-on: https://go-review.googlesource.com/c/go/+/369161
Trust: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants