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

Add new i128 unit tests & patch-01 #92

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BlobMaster41
Copy link
Contributor

7 tests fails that should pass

Proof that the current implementation has major issues


Impact / Severity

Vulnerability Severity
i128 Negative Arithmetic High
i128.fromBits 32-Bit Shift for hi2 High
Incorrect clz with High Bit Set (i128) Medium
Negative Float → i128 Low Bits Zeroed Medium
Sign Extension & Large fromString Performance Low

Patch-01 (fromBits)

1. How Sign Extension Works (i32 → i64)

Let’s say you have a 32-bit signed integer (i32) whose bits look like this:

11111111 11111111 11111111 11111110
  • In 2’s complement, this bit pattern is -2 (i32).
  • If you directly cast this i32 to i64, the language will sign-extend the value to 64 bits. That means it copies the leftmost bit (the sign bit, 1 in this case) to the newly added high bits.

As a result, the 64-bit (i64) pattern becomes:

11111111 11111111 11111111 11111111 11111111 11111111 11111111 11111110
^ repeated sign bits   ^ original 32 bits

Thus, in decimal, this is still -2 as an i64, which might not be what you want if you needed the original bits as-is.


2. How Zero Extension Works (i32 → u32 → i64)

If your goal is to preserve the exact 32 bits without interpreting them as signed, the usual trick is:

  1. Cast i32u32: Since u32 is an unsigned 32-bit integer, no sign extension occurs. The bit pattern is treated as a non-negative number in 32 bits.

  2. Then cast u32i64 (or u64i64), which zero-extends to 64 bits. It fills the newly created high bits with 0 instead of copying the sign bit.

Following the same example (-2 in i32, which is 11111111 11111111 11111111 11111110):

  • i32 (-2) → u32:
    The same bit pattern reinterpreted as unsigned yields

    11111111 11111111 11111111 11111110  (decimal: 4294967294)
    

    but this time it’s considered u32, not i32. There’s no negative concept here—just bits.

  • u32 → i64:
    When you cast this u32 to an i64, the language zero-extends, producing:

    00000000 00000000 00000000 00000000 11111111 11111111 11111111 11111110
                     ^ zero-extended 32 bits ^
    

    Now, numerically, this is 4294967294 as an i64, which exactly preserves the original lower 32 bits without sign extension.

In other words, you get a value that strictly matches the original 32 bits but in a 64-bit container.


3. Why This Matters

  • Bitwise operations / low-level code: When you’re manipulating lower 32 bits, you might not want negative values or sign bits creeping into the high portion of a 64-bit register.
  • Consistency: Casting i32 to u32 first forces the language to treat those 32 bits as an unsigned quantity. Any further expansion to 64 bits will keep them exactly as they appear.
  • Avoid unintended sign extension: Directly going i32 → i64 can lead to undesired negative values if the sign bit of i32 was 1.

4. Masking Alternative: & 0xFFFFFFFF

Another common technique is using a bitmask:

let x: i32 = ...;
let extended = <i64>(x & 0xFFFFFFFF);

This manually zeros out any higher bits when interpreted as signed, effectively doing the same job as i32 → u32 → i64. Under the hood:

  1. x & 0xFFFFFFFF chops x down to 32 bits in an unsigned sense.
  2. Casting to i64 afterward prevents sign extension, because the upper 32 bits are now zero.

But conceptually, doing i32 → u32 → i64 or x & 0xFFFFFFFFi64 is the same idea: no sign extension—just the raw 32 bits in a bigger integer.

@BlobMaster41
Copy link
Contributor Author

I will create 6 more PR to fix the last 6 test that fails that I added. This PR is strictly for only patching the fromBits method.

@BlobMaster41 BlobMaster41 changed the title Add new i128 unit tests Add new i128 unit tests & patch-01 Jan 20, 2025
@MaxGraey
Copy link
Owner

Thanks! Can you comment out the failed tests for now and uncomment them in future PRs that fix the particular test case?

Comment on lines 85 to +88
static fromBits(lo1: i32, lo2: i32, hi1: i32, hi2: i32): i128 {
return new i128(
<u64>lo1 | ((<u64>lo2) << 32),
<i64>hi1 | ((<i64>hi2) << 32),
);
let lo = ((<u64>lo2) << 32) | (<u64><u32>lo1);
let hi = ((<i64>hi2) << 32) | (<i64><u32>hi1);
return new i128(lo, hi);
Copy link
Owner

Choose a reason for hiding this comment

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

Use i32 for args was a mistake from my side. I guess make sense to replace all types to u32:

  static fromBits(lo1: u32, lo2: u32, hi1: u32, hi2: u32): i128 {
  }

@BlobMaster41
Copy link
Contributor Author

Hey, sorry, I will make the rest of the PRs as soon as possible, I am pretty busy this week but should have some time this weekend

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.

2 participants