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

fix i256 key type conversion by initialize buffer before usage #1252

Merged
merged 2 commits into from
Jun 10, 2023

Conversation

learnforpractice
Copy link
Contributor

fix i256 key type conversion by initialize buffer before usage

export_bits doesn't fill every byte in the buffer, need to ensure that 'buffer' is initialized before usage:

https://github.com/boostorg/multiprecision/blob/5f81a78cdb38ea35f98ef7a655d2535773c1261b/include/boost/multiprecision/cpp_int/import_export.hpp#L236

memset(buffer, 0, 32);
boost::multiprecision::export_bits(v, buffer, 8, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix, good catch! It would definitely work fine.
A thought, since export_bits returns the pointer to the byte just after the last one written, we can do a small optimization by zeroing only the remaining bytes.

Suggested change
memset(buffer, 0, 32);
boost::multiprecision::export_bits(v, buffer, 8, false);
auto out = boost::multiprecision::export_bits(v, buffer, 8, false);
memset(out, 0, 32 - (out - buffer)); // zero out remaining bytes

Copy link
Member

Choose a reason for hiding this comment

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

My hunch is the original is faster (although personally I'd prefer the uint8_t buffer[32] = {} style) because the call to memset() is completely removed (and there isn't a read dependency on the register).

https://godbolt.org/z/axGa6e4bK

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no issue with @spoonincode 's suggestion. Hard to know which version would be fastest without testing, but it probably doesn't matter in this instance.

Copy link
Member

Choose a reason for hiding this comment

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

I'll add my vote for the more c++ style of uint8_t buffer[32] = {}

@greg7mdp greg7mdp self-requested a review June 8, 2023 21:27
@learnforpractice
Copy link
Contributor Author

Updated. Thanks for the suggestion!

@greg7mdp
Copy link
Contributor

greg7mdp commented Jun 9, 2023

@learnforpractice you should be able to just push the merge button to merge your PR.

@spoonincode
Copy link
Member

thanks @learnforpractice

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.

4 participants