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

libutil/veb: fix vebpred() corner case #2342

Merged
merged 3 commits into from Aug 27, 2019

Conversation

@garlick
Copy link
Member

commented Aug 27, 2019

The bug in libveb was actually pretty easy to find - sorry for declaring it hopeless yesterday! It was just a left shift, with shift width equal to the word size (undefined behavior, which on x86_64 was not what the code expected). There's more detail in the commit message. This PR fixes the bug, reverts the idset workaround in #2340, and takes away the TODO on the formerly failing veb test.

See also: https://stackoverflow.com/questions/3871650/gcc-left-shift-overflow

garlick added 3 commits Aug 27, 2019
Problem: vebpred(M-1) at M=32 always returns M.

The root cause is left shifting a 32-bit word by the word size,
which is undefined behavior.  Specifically:  on x86_64 with gcc,
in zeros(), ~0<<32 evaluates to ~0 not 0 as expected by
vebpred() -> ones() -> zeros().

Presumably this is not reported by -Wshift-count-overflow
b/c the shift count is computed, not constant.

Special case k==WORD in zeros(), so that the expected result
is returned.

Add assertions in this and a few other areas where the shift
count is computed to ensure it doesn't overflow.

Fixes #2336

See also: mrdomino/libveb#1
@garlick garlick force-pushed the garlick:vebfix branch from f4fe4f7 to a8f74c5 Aug 27, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Aug 27, 2019

Codecov Report

Merging #2342 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2342      +/-   ##
==========================================
+ Coverage   80.83%   80.85%   +0.01%     
==========================================
  Files         215      215              
  Lines       34240    34252      +12     
==========================================
+ Hits        27677    27693      +16     
+ Misses       6563     6559       -4
Impacted Files Coverage Δ
src/common/libidset/idset.c 96.35% <ø> (-0.11%) ⬇️
src/common/libutil/veb.c 98.95% <100%> (+0.09%) ⬆️
src/common/libflux/mrpc.c 87.79% <0%> (-1.19%) ⬇️
src/cmd/flux-module.c 83.72% <0%> (-0.24%) ⬇️
src/common/libflux/message.c 81.07% <0%> (+0.12%) ⬆️
src/modules/connector-local/local.c 74.42% <0%> (+1.01%) ⬆️
@grondo

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

Nice work! "easy to find" is relative...

@garlick

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

Well I didn't have to read the veb paper to understand the part of the code that wasn't working right at least :-)

@grondo

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

Ok to merge this now? Then at least historically it will follow the other idset fix.

@garlick

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2019

sure.

@grondo grondo merged commit aee64cf into flux-framework:master Aug 27, 2019
4 checks passed
4 checks passed
Summary 1 potential rule
Details
codecov/patch 100% of diff hit (target 80.83%)
Details
codecov/project 80.85% (+0.01%) compared to 802c2cc
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick garlick referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.