jsoniter-scala icon indicating copy to clipboard operation
jsoniter-scala copied to clipboard

INT128 jsoniter_scala_multiply_high() may have problem when one argument is negative.

Open LeeTibbert opened this issue 4 months ago • 2 comments

Recent changes to jsoniter-scala-core/native/src/main/resources/scala-native/multiply_high.c have advanced the discussion.

I do have a concern about one section of code. Please forgive me if I have missed something, either obvious or subtle.

I have not had time to implement a test program, but it seems to me that the INT128 jsoniter_scala_multiply_high() code may have a problem/defect/bug when either (but probably not both) arguments are negative.

Later, Later: Removed wrong comments about discarded upper bits, those are preserved. Wake up, Lee.

#elif defined(__SIZEOF_INT128__)

long long jsoniter_scala_multiply_high(long long x, long long y) {
  return x * (unsigned __int128) y >> 64;

// Careful with undefined behavior of >>. Probably need it #ifdef compiler being known to use arithmitic shift (GCC or Clang (or Apple?))
}

unsigned long long jsoniter_scala_unsigned_multiply_high(unsigned long long x, unsigned long long y) {
  return x * (unsigned __int128) y >> 64;
}

I suspect the code in the #else Hacker's delight branch might have similar issues, but have not had time to study it or do a executable design study

long long t = x1 * y0 + ((unsigned long long) (x0 * y0) >> 32);

// I think for most contemporary compilers a right shift of an unsigned will fill vacated bits with 0.
// I doubt that is defined behavior.
// I am pretty sure that right shifting of a _signed_ value is Undefined Behavior, even in C 23. 

Thank you for your time considering this.

LeeTibbert avatar Jun 10 '25 01:06 LeeTibbert