Skip to content

Attempt at fixing mpmc_bounded_queue on 32bit where integer overflow is a concern. #10258

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

Closed

Conversation

toffaletti
Copy link
Contributor

I'm not super happy with this, I think I've probably missed some edge case and the logic is not at all straight forward. Maybe I should put the overflow logic into its own 'else if' and heavily comment it? Suggestions welcome.

P.S. - I couldn't actually compile this code because master doesn't build for me for some reason. I get:

error: linking with `cc` failed with code 1
note: cc arguments: -L/home/jason/rust/x86_64-unknown-linux-gnu/stage1/lib/rustc/x86_64-unknown-linux-gnu/lib -m64 -o x86_64-unknown-linux-gnu/stage1/lib/rustc/x86_64-unknown-linux-gnu/lib/librustdoc-a437806a76c5f37a-0.9-pre.so x86_64-unknown-linux-gnu/stage1/lib/rustc/x86_64-unknown-linux-gnu/lib/lib.o -L/home/jason/rust/x86_64-unknown-linux-gnu/stage1/lib/rustc/x86_64-unknown-linux-gnu/lib -lstd-6c65cf4b443341b1-0.9-pre -L/home/jason/rust/x86_64-unknown-linux-gnu/stage1/lib/rustc/x86_64-unknown-linux-gnu/lib -lextra-a7c050cfd46b2c9a-0.9-pre -L/home/jason/rust/x86_64-unknown-linux-gnu/stage1/lib/rustc/x86_64-unknown-linux-gnu/lib -lsyntax-64629f7f0c6a9bc-0.9-pre -L/home/jason/rust/x86_64-unknown-linux-gnu/stage1/lib/rustc/x86_64-unknown-linux-gnu/lib -lrustc-d3cb8c2ccd84a7a7-0.9-pre -lsundown -Lx86_64-unknown-linux-gnu/rt/sundown -L/home/jason/rust/.rust -L/home/jason/rust -shared -lrt -ldl -lm -lmorestack -lrustrt -Wl,-rpath,$ORIGIN/. -Wl,-rpath,/home/jason/rust/x86_64-unknown-linux-gnu/stage1/lib/rustc/x86_64-unknown-linux-gnu/lib -Wl,-rpath,/usr/local/lib/rustc/x86_64-unknown-linux-gnu/lib
note: cc: error: x86_64-unknown-linux-gnu/stage1/lib/rustc/x86_64-unknown-linux-gnu/lib/lib.o: No such file or directory

error: aborting due to previous error

I've tried make clean and ./configure again as well as rm -rf tmp x86_64-unknown-linux-gnu.

@thestinger
Copy link
Contributor

There are checked_{mul,sub,add} methods if you just need to do multiplication/addition/subtraction with an overflow check.

@huonw
Copy link
Member

huonw commented Nov 4, 2013

(@alexcrichton is fixing the build failure as we speak.)

@@ -34,7 +34,8 @@ use option::*;
use vec;
use clone::Clone;
use kinds::Send;
use num::{Exponential,Algebraic,Round};
use uint;
use uint::next_power_of_two;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just use uint::next_power_of_two rather than importing it specially?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easy enough

@toffaletti
Copy link
Contributor Author

I feel better about this now. The changes are commented and I think the checks are more clear.

@catamorphism
Copy link
Contributor

@brson @toffaletti Is this ready to merge?

@toffaletti
Copy link
Contributor Author

@catamorphism I think it is ready now.

@brson
Copy link
Contributor

brson commented Nov 18, 2013

I still don't understand this change, and I think somebody needs to in order to merge it.

@alexcrichton
Copy link
Member

As with @brson, I'm uncomfortable merging this when it appears that there aren't that many people understanding what's going on here.

I would be comfortable landing this is someone else can be convinced of the correctness of the overflow handling, or possibly also if tests were added which exposed the bug as it stands today and shows that the changes fix it.

@alexcrichton
Copy link
Member

Closing due to a lack of activity, but as always feel free to reopen with updates!

@toffaletti
Copy link
Contributor Author

Sorry, I've been out of the country for a few weeks. Can revisit this if it ever becomes an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants