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

Jump to bottom

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

Closed
mknyszek opened this issue Dec 3, 2021 · 8 comments
Closed

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

mknyszek opened this issue Dec 3, 2021 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@mknyszek
Copy link
Contributor

mknyszek commented Dec 3, 2021 •
edited

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 3, 2021
@mknyszek mknyszek added this to the Backlog milestone Dec 3, 2021
@cherrymui
Copy link
Member

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

@heschi
Copy link
Contributor

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
Member

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

@heschi
Copy link
Contributor

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
Member

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

@cherrymui
Copy link
Member

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

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
@cherrymui
[dev.boringcrypto] crypto/internal/boring: add -pthread linker flag
21fa0b2 
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>
@qmuntal qmuntal mentioned this issue Jan 10, 2022
Closed
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 8, 2022
@dmitshur dmitshur modified the milestones: Backlog, Unreleased Mar 8, 2022
@dmitshur
Copy link
Contributor

dmitshur commented Mar 8, 2022

I understand this is resolved via https://go.dev/cl/369161, it just didn't get closed because the "Fixes" syntax doesn't close issues on dev.* branches. Notably, https://build.golang.org/?branch=dev.boringcrypto (as well as 1.17 and 1.16 ones) is green.

@dmitshur dmitshur closed this as completed Mar 8, 2022
@golang golang locked and limited conversation to collaborators Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@mknyszek @dmitshur @gopherbot @cherrymui @heschi