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

Bounds inference handles casts from uint32 to int32 incorrectly #7807

Open
abadams opened this issue Aug 24, 2023 · 4 comments · May be fixed by #7814
Open

Bounds inference handles casts from uint32 to int32 incorrectly #7807

abadams opened this issue Aug 24, 2023 · 4 comments · May be fixed by #7814
Assignees
Labels

Comments

@abadams
Copy link
Member

abadams commented Aug 24, 2023

See https://github.com/halide/Halide/blob/main/src/Bounds.cpp#L284

It assumes they can't overflow, but we recently changed that from UB to wrapping semantics in #7769

Now I'm not clear if a case that would have overflowed would have compiled correctly previously (because it would have created a UB expression inside bounds inference), but now it's definitely not right. Here's a failure case that segfaults instead of throwing a bounds error:


#include "Halide.h"

using namespace Halide;

int main(int argc, char **argv) {
    Var x;
    Func f, g, h;
    Param<uint32_t> p;

    f(x) = x;

    g(x) = f(x) + f(x + p);

    f.compute_root();

    p.set(0x80000000U);
    g.realize({128});

    return 0;
}

@abadams abadams added the bug label Aug 24, 2023
@abadams
Copy link
Member Author

abadams commented Aug 24, 2023

Actually my example isn't quite right, because the addition is signed, so the cast of p to int32_t (which is well-defined) happens before the addition, so really what this is a signed integer overflow inside bounds inference that has nothing to do with uint32s. Sigh.

@abadams
Copy link
Member Author

abadams commented Aug 24, 2023

At root this is the same issue as #3245

@steven-johnson
Copy link
Contributor

Here's a failure case that segfaults instead of throwing a bounds error:

...does this mean the current state of main is dodgy? (I had just started a GitHub -> Google merge, but will abandon it if this means we might be injecting badness)

@abadams
Copy link
Member Author

abadams commented Aug 25, 2023

Should have no impact on working code. It was always possible to produce crashing code by tricking bounds inference into doing signed integer overflow. You have to really try though. I think which of these cases will throw a compile-time error vs which will crash at runtime might have changed with the change to cast constant folding.

At the very least, the comments are wrong, because casting from a uint32 to an int32 is well-defined now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants