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.

Sign up for GitHub

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

runtime: de-duplicate bit operations with math/bits #35569

Open
aclements opened this issue Nov 13, 2019 · 6 comments
Open

runtime: de-duplicate bit operations with math/bits #35569

aclements opened this issue Nov 13, 2019 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Backlog

Comments

@aclements
Copy link
Member

For the page allocator rewrite, @mknyszek tried to depend on math/bits in the runtime. This almost works, but conflicts with go test -coverpkg=all (#35461) because that tries to instrument math/bits in ways that are incompatible with running inside the runtime. In order to get this working, CL 206199 duplicated some math/bits functions into the runtime (and CL 206200 intrinsified them).

This duplication was expedient, but unfortunate, and for 1.15 I'd like to reconsider this. A few possibilities:

  1. -coverpkg=all shouldn't apply to anything the runtime depends on. You'd hardly be losing anything if it didn't cover math/bits since almost all of those functions are intrinsified anyway.

  2. If the cover tool can switch to using compiler-inserted coverage information, rather than source rewriting, it's possible this problem will go away.

/cc @dr2chase @cherrymui @mdempsky
@aclements aclements added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 13, 2019
@aclements aclements added this to the Go1.15 milestone Nov 13, 2019
@cherrymui
Copy link
Member

If the cover tool can switch to using compiler-inserted coverage information, rather than source rewriting, it's possible this problem will go away.

The failure only happens when both -coverpkg=all and the race detector are enabled. The cover tool inserts a call to sync/atomic.AddUint32 (or like), which is further instrumented by the race detector, making it a call to the race detector code, when it doesn't have a P (e.g. in schedinit).

It might work fine if it is just an atomic counter increment.

@mdempsky
Copy link
Contributor

Unifying with fuzzing instrumentation sounds good to me, and would avoid the dependency loop. I can play around with this during the freeze.

@odeke-em
Copy link
Member

Howdy Austin, Cherry and Matthew? I shall move this to Go1.16, but please feel free to adjust it as you please.

@odeke-em odeke-em modified the milestones: Go1.15, Go1.16 May 25, 2020
@aclements
Copy link
Member Author

Thanks @odeke-em . @mdempsky, just curious if you had any opportunity to look at unifying the fuzzing instrumentation.

@mdempsky
Copy link
Contributor

@aclements No, not yet.

@aclements aclements modified the milestones: Go1.16, Go1.17 Dec 8, 2020
@ianlancetaylor
Copy link
Member

Moving to Backlog.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.17, Backlog Apr 21, 2021
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
@mknyszek mknyszek moved this to Triage Backlog in Go Compiler / Runtime Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Go Compiler / Runtime
Status: Triage Backlog
Milestone
Backlog
Development

No branches or pull requests
6 participants
@mdempsky @aclements @ianlancetaylor @odeke-em @gopherbot @cherrymui