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

Signed integer overflow _fnlHash2D #140

Open
Vaxeral opened this issue May 13, 2024 · 3 comments
Open

Signed integer overflow _fnlHash2D #140

Vaxeral opened this issue May 13, 2024 · 3 comments

Comments

@Vaxeral
Copy link

Vaxeral commented May 13, 2024

Hi. The library seems to have undefined behavior, specifically signed integer overflow.
include/FastNoiseLite.h:484:10: runtime error: signed integer overflow: 1337 * 668265261 cannot be represented in type 'int'

static inline int _fnlHash2D(int seed, int xPrimed, int yPrimed)
{
    int hash = seed ^ xPrimed ^ yPrimed;

    hash *= 0x27d4eb2d;
    return hash;
}

Is this intended, if so what is the reason?

@Auburn
Copy link
Owner

Auburn commented May 14, 2024

The overflow is intended it's part of the hashing algorithm

@Vaxeral
Copy link
Author

Vaxeral commented May 14, 2024

Right and it will work almost always.
There are cases where signed integer overflow being undefined behavior can cause confusion.

int foo(int x) {
    return x+1>x;
}

The optimizer will take advantage of this fact and return 1 regardless of the value x + 1 which may overflow.

But more importantly direclty importing the c library into a zig code base is affected by this issue. Because zig is very strict about violating undefined behavior the example fails in debug builds. This can easily be fixed on zig side by using the overflow operator. Just thought I'd share.

@ForeverZer0
Copy link
Contributor

While it technically this is undefined behavior because it is not an unsigned integer, realistically this library only utilizes well-defined behavior, specifically the "wrapping" rules for shifting, addition. subtraction, and multiplication. According to the spec regarding overflow of signed integers:

C99 6.2.6.2:2:

If the sign bit is one, the value shall be modified in one of the following ways:
— the corresponding value with sign bit 0 is negated (sign and magnitude);
— the sign bit has the value −(2N) (two’s complement);
— the sign bit has the value −(2N − 1) (one’s complement).

So this means it is well-defined behavior for any processor that uses two's complement representation, simply not well-defined behavior per the "letter of the law" of the C-standard as a whole. In the real-world, this means all processors that this library would ever be used with. It would be a challenge to even find an an architecture that doesn't use two's complement, even in the realm of micro-controllers and embedded systems it would be exotic. Suffice it to say, this "undefined behavior" can safely be ignored/suppressed.

That said, I am in the final stages of a Zig implementation that I intend to contribute, and everything works as expected. There were no undefined behavior being utilized that could not be translated, only the additional verbosity of Zig in order to explicitly convey the intent of overflow, which is standard for any C-to-Zig port.

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

No branches or pull requests

3 participants