Halide icon indicating copy to clipboard operation
Halide copied to clipboard

Bounds inference handles casts from uint32 to int32 incorrectly

Open abadams opened this issue 2 years ago • 4 comments

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 avatar Aug 24 '23 23:08 abadams

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 avatar Aug 24 '23 23:08 abadams

At root this is the same issue as #3245

abadams avatar Aug 24 '23 23:08 abadams

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)

steven-johnson avatar Aug 25 '23 00:08 steven-johnson

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.

abadams avatar Aug 25 '23 00:08 abadams