Small Bugfix: Remove possibility of bitshifting negative integers

parthenon-hpc-lab / parthenon

Parthenon AMR infrastructure

https://parthenon-hpc-lab.github.io/parthenon/

Other

105 stars 33 forks source link

Small Bugfix: Remove possibility of bitshifting negative integers #1111

Closed lroberts36 closed 2 weeks ago

lroberts36 commented 3 weeks ago

PR Summary

This PR fixes the bitshift related undefined behavior described in #1107 by multiplying by a power of two explicitly rather than performing the same operation (on positive numbers) via a bitshift.

PR Checklist

[ ] Code passes cpplint
[ ] New features are documented.
[ ] Adds a test for any bugs fixed. Adds tests for new features.
[ ] Code is formatted
[ ] Changes are summarized in CHANGELOG.md
[ ] Change is breaking (API, behavior, ...)
- [ ] Change is additionally added to CHANGELOG.md in the breaking section
- [ ] PR is marked as breaking
- [ ] Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
[ ] CI has been triggered on Darwin for performance regression tests.
[ ] Docs build
[ ] (@lanl.gov employees) Update copyright on changed files

pgrete commented 3 weeks ago

btw you got the Schnapszahl ;)

lroberts36 commented 3 weeks ago

@pgrete: I am not sure why this worked previously, but I did not look carefully at what happened with the negative but shift. I think this should have been hit reasonably often.

Haha, I had to look up schnapszahl...

pgrete commented 2 weeks ago

The test now reliably failed twice. Does anyone see how this changeset could result in that error? I don't.

BenWibking commented 2 weeks ago

The test now reliably failed twice. Does anyone see how this changeset could result in that error? I don't.

It's a NumPy 2.0 API breakage 😞...

AttributeError: module 'numpy' has no attribute 'product'

BenWibking commented 2 weeks ago

The test scripts need to be updated: https://numpy.org/devdocs/numpy_2_0_migration_guide.html#numpy-2-migration-guide

lroberts36 commented 2 weeks ago

@pgrete and @BenWibking: Apparently this worked because (at least clang and gcc) bitshift negative numbers in the expected way, i.e neg_num << n == neg_num * std::pow(2, n).

BenWibking commented 2 weeks ago

CI fix here: https://github.com/parthenon-hpc-lab/parthenon/pull/1116

pgrete commented 2 weeks ago

@pgrete and @BenWibking: Apparently this worked because (at least clang and gcc) bitshift negative numbers in the expected way, i.e neg_num << n == neg_num * std::pow(2, n).

great, thanks for confirming!