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

Make amino build on 32bit architectures #242

Merged
merged 4 commits into from
Nov 8, 2018
Merged

Conversation

liamsi
Copy link
Contributor

@liamsi liamsi commented Nov 5, 2018

Some constants in encoder.go are interpreted as int by default and they overflow ints on 32bit machines: encoder.go
Similarly some values that seed the fuzzer: here for instance.

The first makes tendermint builds fail on certain architectures. This PR is a quick-fix for this issue.
The changes made in this PR were tested (and fuzzed) with a i686 docker image (and locally for 64bit).

@liamsi liamsi changed the base branch from master to develop November 5, 2018 15:11
 - explicitly type constant in time encoding
 - use math.MaxInt32 in int tests to seed the fuzzer
@liamsi liamsi changed the title WIP: Make amino build on 32bit architectures Make amino build on 32bit architectures Nov 6, 2018
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

🍇

encoder.go Outdated Show resolved Hide resolved
encoder.go Outdated Show resolved Hide resolved
melekes and others added 2 commits November 6, 2018 17:15
Co-Authored-By: Liamsi <Ismail.Khoffi@gmail.com>
Co-Authored-By: Liamsi <Ismail.Khoffi@gmail.com>
Copy link

@greg-szabo greg-szabo left a comment

Choose a reason for hiding this comment

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

Looks good to me, but weren't there similar settings in decoder.go? Seems that only encoder.go was fixed.

@liamsi
Copy link
Contributor Author

liamsi commented Nov 7, 2018

Thanks for the review @greg-szabo. I think we were seeing errors in encoder.go because it was using that same variables defined in decoder.go. After #243 gets approved, I'll tag another release, which should make the arm/32bit builds work again.

@liamsi liamsi merged commit 2cbe6f0 into develop Nov 8, 2018
@liamsi liamsi deleted the build_on_32bit_again branch November 8, 2018 15:30
liamsi added a commit that referenced this pull request Nov 11, 2018
* add contribution guidelines

- minimal changes to ensure proto3 compat. for unsigned ints (int,
int32, int64)

* Add changes to decode ints, too

* Delete outdated tests (covered by new table driven test)

* Add overflow checks for int & int32

* prep release: update changelog

* Make amino build on 32bit architectures (#242)

* fix int overflows that happen on 32bit systems
 - explicitly type constant in time encoding
 - use math.MaxInt32 in int tests to seed the fuzzer

* Add --concrete-name option to aminoscan
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.

3 participants