Skip to content

Conversation

@sipa
Copy link
Contributor

@sipa sipa commented Nov 16, 2022

This is a follow-up to #1000:

  • Add randomized unit tests for int128 logic.
  • Add CI for the _(u)mulh code path (on non-ARM64 MSVC).
  • Add heuristic logic to enable int128_struct based arithmetic on 64-bit MSVC, or systems with pointers wider than 32 bits.
  • Fix signed overflow in ARM64 MSVC code.

}

static SECP256K1_INLINE void secp256k1_i128_load(secp256k1_int128 *r, int64_t hi, uint64_t lo) {
*r = (((uint128_t)(uint64_t)hi) << 64) + lo;
Copy link
Contributor

Choose a reason for hiding this comment

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

why not ((int128_t)hi << 64) + lo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

((int128_t)hi << 64) + lo is UB if hi is negative (left-shift of negative value is undefined).

((uint128_t)hi << 64) + lo would work, but involves sign-extending hi, and then throwing those extension bits away.

((uint128_t)(uint64_t)hi << 64) + lo is equivalent, but avoids the sign-extension, which I thought would be more obviously correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow I missed the fact the left-shift of negative values is UB. In retrospect, that makes a bit of sense if you cannot count on two's complement, even if the formal definition of multiplying by 2^n still makes sense for negative numbers.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

utACK 3afce0a

@roconnor-blockstream
Copy link
Contributor

utACK 99bd335

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

utACK 99bd335

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

utACK 99bd335

EDIT: Maybe #1158 (comment) should be merged first?

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

ACK 99bd335 tested this also on MSVC locally with the override, including all the benchmark binaries

EDIT: Maybe #1158 (comment) should be merged first?

I think we should be greedy and merge it now that it has enough ACKs. The other one will be rather easy to rebase.

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