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

Build intx bignum library in CMake #245

Closed
wants to merge 6 commits into from
Closed

Build intx bignum library in CMake #245

wants to merge 6 commits into from

Conversation

jakelang
Copy link
Member

@jakelang jakelang commented May 29, 2018

Fixes #190

@axic
Copy link
Member

axic commented May 31, 2018

@chfast can you review this?

Copy link
Collaborator

@chfast chfast left a comment

Choose a reason for hiding this comment

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

  • use latest master (1a10f4fc3433d5ce88ea4c6067002680e2ea0385)
  • build and install steps should be needed to modify
  • add cmake arg INTX_TESTING=OFF

@jakelang
Copy link
Member Author

@chfast INTX_TESTING doesn't exist on latest master branch yet.

@jakelang
Copy link
Member Author

@chfast just opened a PR on intx to make said option.

@jakelang
Copy link
Member Author

jakelang commented Jun 1, 2018

badab6e seems to be causing ninja to break in Debug mode with this error message:
ninja: error: 'intx::intx-NOTFOUND', needed by 'src/libhera.so', missing and no known rule to make it
I tried building with RelWithDebInfo mode as well as None which sticks to existing compiler options but these cause the compiler to fail with
src/CMakeFiles/hera.dir/build.make:147: *** target pattern contains no '%'. Stop.

@chfast
Copy link
Collaborator

chfast commented Jun 1, 2018

In Ninja you have to specify "byproducts" (as you did). Maybe some path there is incorrect. I would stick to Make because it's easier to start with.

For Make error, you have to check out what's in hera.dir/build.make:147.

@@ -974,11 +977,11 @@ inline int64_t maxCallGas(int64_t gas) {
ensureCondition(safeLoadUint128(balance) >= safeLoadUint128(value), OutOfGasException, "Out of gas.");
}

uint64_t EthereumInterface::safeLoadUint128(evmc_uint256be const& value)
intx::uint128 EthereumInterface::safeLoadUint128(evmc_uint256be const& value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You know that GCC / clang have unsigned __int128?

Copy link
Member

Choose a reason for hiding this comment

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

@chfast would have been cool if you could have mentioned that when the original issue was created :)

But according to https://gcc.gnu.org/onlinedocs/gcc/_005f_005fint128.html :

As an extension the integer scalar type __int128 is supported for targets which have an integer mode wide enough to hold 128 bits.
There is no support in GCC for expressing an integer constant of type __int128 for targets with long long integer less than 128 bits wide.

Copy link
Member Author

Choose a reason for hiding this comment

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

So we can assume it works for x86-64 but not much else until we investigate

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wasn't aware that 128-bit is the max precision you need.

Anyway, the type is available on 64-bit targets. Because you don't support MSVC at the moment it can be used as a temporary solution.

@jakelang jakelang self-assigned this Jun 1, 2018
@axic axic closed this in #247 Jun 2, 2018
@axic axic removed the in review label Jun 2, 2018
@axic axic deleted the intx branch November 29, 2018 10:21
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