Add support for tuning spec path via C API#184
Conversation
0bef8b3 to
3ca2722
Compare
610c96c to
cd67720
Compare
|
I donot think the CI error is caused by changes from this PR. |
cd67720 to
47ab2f5
Compare
This is true, but your change to remove I looked at the violations and they are in external libraries and we might need to add suppressions - RCCL suppression in |
32fa38a to
ec92085
Compare
|
Here's the IREE issue and minimal repro for the RCCL leak: iree-org/iree#23561 Please add to the existing With this suppression, ASAN is fixed but also I don't see the UBSAN error anymore (it might have been a cascade effect). |
I dug into this and found an interesting anecdote: UBSAN defaults to continuing (not aborting) on violation (unlike ASAN). So unless something else fails, the UBSAN reports are easy to miss - they don't generate non-zero return code and don't break CI. That's what happened above, the ASAN error broke CI but the logs contained UBSAN violations too. We do force it to abort in Fusilli (by setting |
## Summary @bangtianliu 's recent [PR](#184) switches the benchmark runner to use the C-API compile backend (instead of CLI) - this exposed a few external LSAN and UBSAN [issues](https://github.com/iree-org/fusilli/actions/runs/22331991157/job/64616213364) in IREE that the subprocess CLI invocation was masking. I've reduced them to minimal self contained reproducers and filed issues in IREE for tracking: 1. **Memory leak in HIP driver RCCL initialization** ([iree-org/iree#23561](iree-org/iree#23561)) - ~1216 bytes leaked during `dlopen(librccl.so)` in IREE HIP driver - Occurs on every driver creation 2. **NULL pointer to memcpy** ([iree-org/iree#23562](iree-org/iree#23562)) - Undefined behavior at `hip_device.c:2561` - Triggered during host-to-device buffer transfers with empty binding tables - Violates `memcpy` nonnull attribute contract While looking into the above, I realized an oddity with UBSAN: UBSAN defaults to continuing (not aborting) on violation (unlike ASAN). So unless something else fails, the UBSAN reports are easy to miss - they don't generate non-zero return code and don't break CI. In the case above, ASAN broke CI but the logs contained UBSAN violations too. We do force abort in Fusilli (by setting `-fno-sanitize-recover=undefined`) but unfortunately IREE's UBSAN config doesn't do that. Therefore to force abort on UBSAN issues within IREE, we've got to set `UBSAN_OPTIONS=halt_on_error=1`. This PR configures UBSAN with strict error handling (`halt_on_error=1`) and suppressions to properly handle undefined behavior in IREE's HIP runtime. This enables catching new bugs immediately while suppressing known upstream IREE issues. ## Changes - **Add UBSAN suppressions file** (`build_tools/sanitizers/ubsan_suppressions.txt`) - Suppresses NULL pointer to memcpy bug in IREE HIP device (iree-org/iree#23562) - **Configure UBSAN_OPTIONS across all test types** - Benchmark tests (`benchmarks/CMakeLists.txt`) - Lit tests (`build_tools/cmake/FusilliTestUtils.cmake`) - CTest targets (`build_tools/cmake/FusilliTestUtils.cmake`) - Sets `halt_on_error=1:print_stacktrace=1` to abort on UB violations - **Add LSAN suppression for RCCL leak** (iree-org/iree#23561) - Suppresses memory leak during `iree_hal_hip_nccl_dynamic_symbols_initialize()` - **Update README** with sanitizer configuration documentation - **Add `-fno-sanitize-merge`** flag to preserve source locations in UBSAN reports **Assisted-by:** Claude Opus 4.6 --------- Signed-off-by: Sambhav Jain <sambhav@alumni.stanford.edu> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
ec92085 to
0bb96ce
Compare
| REQUIRE(std::filesystem::exists(output.path)); | ||
| REQUIRE(std::filesystem::file_size(output.path) > 0); | ||
|
|
||
| std::filesystem::remove_all(input.path.parent_path()); |
There was a problem hiding this comment.
will this be cleaned up if any of the require checks fail above?
There was a problem hiding this comment.
Good catch!
I think you're pointing to a design pattern issue used across multiple test files (test_cache.cpp, test_compile_command.cpp, and test_compile_session.cpp).
You are right, if any of the REQUIRE() checks fail, the cleanup code never runs.
I see the manual remove_all(parent_path()) calls remove the entire cache directory, while the CacheFile destructor with remove=true only removes individual files. However, the cleanup still won't execute if a REQUIRE() fails.
I'll address this in a separate PR to keep this one focused on the tuning spec path via C-API, Sound good? Also cc @sjain-stanford here.
There was a problem hiding this comment.
Oh, test_cache.cpp already follows the correct pattern (cleanup outside SECTION). I should apply the same pattern to test_compile_session.cpp and test_compile_command.cpp in a separate PR to fix the cleanup issue.
sjain-stanford
left a comment
There was a problem hiding this comment.
LGTM modulo nits. Thanks for re-enabling the C-API path for the benchmark runner - it helped unmask some of the sanitizer issues as well.
b1e88c0 to
ad9d9e6
Compare
662cb2b to
6886004
Compare
6886004 to
3a098a2
Compare
c52499d to
a9f504d
Compare
Signed-off-by: Bangtian Liu <liubangtian@gmail.com>
Address review feedback: replace auto with explicit types for clarity. Signed-off-by: Bangtian Liu <liubangtian@gmail.com>
a9f504d to
b543494
Compare
Signed-off-by: Bangtian Liu <liubangtian@gmail.com>
b543494 to
88f5d10
Compare
Motivated by the issue pointed by reviewer comments: #184 (comment), this PR add `ScopeExit` RAII helper to `tests/utils.h `that removes cache directories via destructor, ensuring cleanup happens even when tests fail. Implementation uses std::function<void()> for simplicity. **Assisted-by:** Claude --------- Signed-off-by: Bangtian Liu <liubangtian@gmail.com>
--iree-codegen-tuning-spec-pathhas been exposed through the C-API by IREE PR iree-org/iree#23404. The PR removes the CLI backend requirement for testing and also update readme accordingly.Assisted-by: Claude