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

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

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

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

aclements opened this issue Nov 13, 2019 · 5 comments
Labels
Milestone

Comments

@aclements
Copy link
Member

@aclements aclements commented Nov 13, 2019

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 this to the Go1.15 milestone Nov 13, 2019
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Nov 13, 2019

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
Member

@mdempsky mdempsky commented Nov 13, 2019

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

@odeke-em odeke-em commented May 25, 2020

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

@aclements aclements commented May 26, 2020

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

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 26, 2020

@aclements No, not yet.

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
You can’t perform that action at this time.