Avoid throwing from a destructor in PartitionLoops.cpp #8767
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcourteaux
approved these changes
Aug 19, 2025
Member
Author
|
Going to investigate the memory leak reported here. |
Member
Author
|
I went in and built llvm-symbolizer (and updated the build master so it will do it automatically going forward) to get stack traces to the leaking allocations... looks like it's finding leaks in other parts of the compiler unrelated to PartitionLoops. Worse, I wasn't able to reproduce the leak on either my Ubuntu 24.04 x86 desktop or my MacBook. On Ubuntu, Valgrind says everything is "still reachable" (some of the LLVM command-line options stuff stores new'd pointers in global, static memory, apparently). I wonder if this is a consequence of throwing in a destructor? Here's the more detailed leak report: Details |
Contributor
It doesn't sound all that crazy that the compiler just gives up on cleanup and calling destructors if a new exception is thrown. You'd get an explosion of combinations of cleanup code where stuff can break if the destructors start throwing too... Although I would argue that this is indeed a desirable feature in this simple scenario. Jai's |
Contributor
|
I suggest wrapping the actual mutation code in a helper function, and doing the error checking for the loop partition mutation manually outside that wrapper, without relying on a destructor. |
Member
Author
Same with Zig, which is a language that actually, y'know, exists 😉
I'd be more comfortable making a large refactoring if I could tell that it actually fixed the problem. I can't reproduce the issue locally yet. |
Member
|
It claims a For node is leaked. The exception is thrown inside a For IRMutator method. Seems like this is probably real. Maybe there's UB here so it's not a guaranteed leak and that's why you can't repro locally? |
Member
|
+1 to Martijn's suggestion, which I think amounts to renaming this method visit_for, having it additionally return if partitioning happened, and doing the checking in visit(const For *). Doing it blind and seeing if it appeases the buildbot sucks but might be the fastest option. |
alexreinking
changed the title
alexreinking
force-pushed
the
bugfix/partition-loops
branch
from
57a4f8a to
69afa21
Compare
Contributor
|
The new Error-marco with for-loop trick didn't fix this problem? |
mcourteaux
requested changes
Aug 22, 2025
Member
Author
Unfortunately, no. Seems like the problem runs deeper. I'm going to build Halide using LLVM main and see if that reproduces the issue. |
Member
Author
|
I built the test on macOS using LLVM 22 (main) and its ASAN-enabled runtime. I only see perfectly spurious leaks (see details below). I'm at a loss as to why leaks are being detected here... Details |
mcourteaux
approved these changes
Aug 22, 2025
Member
Author
|
It appears the test has started working again! I believe this to have been an LLVM main injection all along... https://buildbot.halide-lang.org/master/#/builders/273/builds/60 However, it looks like we checked #8689 in with a failing test. The |
Contributor
Is this because of IRVisitor / IRMutator recursions going too deep? Can we increase stack size further? |
Member
Author
We normally do, but not on ASAN. There's explicit logic in there that disables |
Member
Author
|
Failures are unrelated. The new |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Destructors are
noexcept(true)by default and throwing (asuser_errordoes) in such a function always callsstd::terminate. That made this particular exception non-recoverable in user code.The included test aborts without the patch to PartitionLoops.cpp.