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

Conditional allocations shouldn't fail if cond=false, size=0 -> nullptr result #7255

Open
steven-johnson opened this issue Dec 28, 2022 · 3 comments

Comments

@steven-johnson
Copy link
Contributor

steven-johnson commented Dec 28, 2022

Allocations can be conditional; if the condition evaluates to false, we end up calling halide_malloc(0). Since it's legal via spec for malloc(0) to return nullptr, we need to be cautious here: if we are compiling with assertions enabled, and have a malloc() (etc) implementation that returns nullptr for alloc(0), we need to skip the assertion check, since we know the result won't be used... but the assertion would fail because the (ignored) result is null.

NOTE: I've only seen this in the C++ backend; so far, I can't replicate it with LLVM backends.

@abadams
Copy link
Member

abadams commented Dec 28, 2022

This may be an issue unique to the C backend, if that's where you observed this. I see code that handles this in CodeGen_Posix.cpp:307

@steven-johnson
Copy link
Contributor Author

Yep. That logic looks simpler than what I have for a fix, I'll try replicating that instead. Fix on the way either way.

steven-johnson added a commit that referenced this issue Dec 28, 2022
Allocations can be conditional; if the condition evaluates to false, we end up calling `halide_malloc(0)` (or `halide_tcm_malloc(0)` in the xtensa branch). Since it's legal via spec for `malloc(0)` to return nullptr, we need to be cautious here: if we are compiling with assertions enabled, *and* have a malloc() (etc) implementation that returns nullptr for alloc(0), we need to skip the assertion check, since we know the result won't be used.

Note: a similar check will be inserted in the xtensa branch separately.
Note 2: LLVM backend already has this check via Codegen_Posix.cpp
@steven-johnson
Copy link
Contributor Author

FYI: via inserting a crash at Codegen_Posix.cpp:298 and rebuilding the universe, I can't find a single AOT case in our tests that actually triggers the !is_const_one() case. (Gonna test JIT shortly)

steven-johnson added a commit that referenced this issue Dec 28, 2022
steven-johnson added a commit that referenced this issue Dec 28, 2022
…) (#7256)

* Conditional allocations shouldn't fail for size=0 in C++ backend (#7255)

Allocations can be conditional; if the condition evaluates to false, we end up calling `halide_malloc(0)` (or `halide_tcm_malloc(0)` in the xtensa branch). Since it's legal via spec for `malloc(0)` to return nullptr, we need to be cautious here: if we are compiling with assertions enabled, *and* have a malloc() (etc) implementation that returns nullptr for alloc(0), we need to skip the assertion check, since we know the result won't be used.

Note: a similar check will be inserted in the xtensa branch separately.
Note 2: LLVM backend already has this check via Codegen_Posix.cpp

* Update CodeGen_C.cpp
ardier pushed a commit to ardier/Halide-mutation that referenced this issue Mar 3, 2024
…ide#7255) (halide#7256)

* Conditional allocations shouldn't fail for size=0 in C++ backend (halide#7255)

Allocations can be conditional; if the condition evaluates to false, we end up calling `halide_malloc(0)` (or `halide_tcm_malloc(0)` in the xtensa branch). Since it's legal via spec for `malloc(0)` to return nullptr, we need to be cautious here: if we are compiling with assertions enabled, *and* have a malloc() (etc) implementation that returns nullptr for alloc(0), we need to skip the assertion check, since we know the result won't be used.

Note: a similar check will be inserted in the xtensa branch separately.
Note 2: LLVM backend already has this check via Codegen_Posix.cpp

* Update CodeGen_C.cpp
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

2 participants