Conversation
d7954a4 to
2de764f
Compare
f0400c3 to
43cac67
Compare
9d93cf3 to
9917c1a
Compare
9917c1a to
82c7d73
Compare
sjain-stanford
left a comment
There was a problem hiding this comment.
Thanks for taking this up. I did an initial pass and left some comments, mostly minor.
| -DFUSILLI_SYSTEMS_AMDGPU=ON | ||
| -DFUSILLI_ENABLE_ASAN=ON | ||
| asan-env: >- | ||
| ASAN_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer-18 |
There was a problem hiding this comment.
While it works even now (since symbolizer versions are generally compatible), this job uses clang-22 so I wonder if it'd be more accurate to use llvm-symbolizer-22 if available in the Docker image.
There was a problem hiding this comment.
Yep, I agree and I thought about that. But clang-18 is used from .cache/docker/venv:
root:/opt/fusilli# which clang-22
/workspace/.cache/docker/venv/bin/clang-22
root:/opt/fusilli# ls /workspace/.cache/docker/venv/bin/
Activate.ps1 activate.csh clang++ f2py iree-import-onnx iree-opt numpy-config pip3.12 python3.12
FileCheck activate.fish clang++-22 iree-build iree-ir-tool isympy pip python
activate clang clang-22 iree-compile iree-link lit pip3 python3
But llvm-symbolizer is missed there.
At the same time there are llvm-symbolyzer-18 and clang-18 in /usr/bin in the Docker image.
So please let me know if it's possible to put llvm-symbolyzer-22 to venv of the image. I believe it would be better
There was a problem hiding this comment.
There's an llvm-symbolizer (-22) in .cache/docker/therock/lib/llvm/bin/. I can add the symbolic link to that from .cache/docker/venv/bin (like we do fo clang-22). Will share once done.
There was a problem hiding this comment.
Landed in docker and updated in fusilli. So once you rebase/pull main, you should be able to just reference llvm-symbolizer-22 and llvm-symbolizer-18 in the two workflows respectively.
$ which llvm-symbolizer-22
/home/srajeshk/claude-workspace/.cache/docker/venv/bin/llvm-symbolizer-22
$ which llvm-symbolizer-18
/usr/bin/llvm-symbolizer-18There was a problem hiding this comment.
Great! Thanks a lot for this feature. Now the versions are aligned!
UPD: When I set ASAN_SYMBOLIZER_PATH=llvm-symbolizer-22, ASAN couldn't find it in CI machine
There was a problem hiding this comment.
The trick is to leave it unset - ASAN will find it from $PATH.
| -DFUSILLI_ENABLE_ASAN=ON | ||
| asan-env: >- | ||
| ASAN_SYMBOLIZER_PATH=/usr/bin/llvm-symbolizer-18 | ||
| ASAN_OPTIONS=detect_leaks=1:halt_on_error=0:quarantine_size_mb=0 |
There was a problem hiding this comment.
This is probably to get all ASAN errors in one-shot, but does it fail CI when ASAN detects memory errors? Verify that the ASAN exit code is propagated correctly.
fae10e2 to
5df1948
Compare
5df1948 to
0073c96
Compare
1629fe9 to
0064e9a
Compare
| asan-env: >- | ||
| ASAN_SYMBOLIZER_PATH=/workspace/.cache/docker/venv/bin/llvm-symbolizer-22 | ||
| ASAN_OPTIONS=detect_leaks=1:halt_on_error=0 | ||
| LSAN_OPTIONS=suppressions=/workspace/build_tools/sanitizers/lsan_suppressions.txt |
There was a problem hiding this comment.
Instead of setting ASAN env vars in the CI workflow yml, I'd just bake it in the CMake rules (e.g. in FusilliTestUtils.cmake and benchmark targets) so that local runs are basically just ctest ... without prefixing with env vars. All that the user needs to specify when building is -DFUSILLI_ENABLE_ASAN=ON.
For example _add_fusilli_ctest_target can be updated to include:
if(FUSILLI_ENABLE_ASAN)
list(APPEND _ENV_VARS
"ASAN_OPTIONS=..."
"UBSAN_OPTIONS=..."
"LSAN_OPTIONS=suppressions=${PROJECT_SOURCE_DIR}/build_tools/sanitizers/lsan_suppressions.txt"
)
endif()There was a problem hiding this comment.
Then you can reuse the "Test fusilli (libIREECompiler CAPI)" and/or "Test fusilli (iree-compile CLI)" steps for ASAN as well (no need to add a separate ctest step for ASAN below.
There was a problem hiding this comment.
I was thinking about user experience as well. I mean if a developer tries to launch asan locally, probably he would like to set different ASAN options (enable/disable some checks). And the best way to do it - just set env variable. If we hardcode env variables to executables, it might lead to inconveniences for local development .
Also we should set LLVM_SYMBOLYZER here but the path to this tool might be different in local env and in CI env.
So I'm not sure that this is good idea to bake env variables in cmake... It might lead to inconveniences during local development but manual setting of env variables is not a big deal, I think.
This is just my opinion but if you prefer to bake variables, I will do it. Just let me know :)
There was a problem hiding this comment.
We'll need a custom approach for each option since the ergonomics are different. Let's tackle them one at a time:
- LSAN_OPTIONS / suppressions: This has to be baked in since the suppression file is checked-in to the code. IREE does it this way too (here).
- LLVM_SYMBOLIZER: I am pretty sure ASAN auto-finds
llvm-symbolizerif on$PATH- this should be the case for both CI (with the docker changes to add it to venv) and local env (but users need to make sure it is visible on$PATH). At least as of 2013 it should: https://git.raptorcs.com/git/bcm5719-llvm/commit/compiler-rt/lib?id=5b5c99d219884c4356d0ce47b896ed7c4d098cb2 - ASAN_OPTIONS: You're right that developers might want to tweak these, but a sensible default that the developer can override (by setting the env var in their shell) is strictly better than no default at all. Then CI can use the defaults baked in. Look into CMake's
set_if_undefinedwhich only takes effect when developer hasn't set them in their shell:
In _add_fusilli_ctest_target (build_tools/cmake/FusilliTestUtils.cmake), after the existing set_tests_properties for ENVIRONMENT. The same will be needed in benchmarks but
# Set environment variables for test
set_tests_properties(
${_RULE_NAME} PROPERTIES
ENVIRONMENT "${_ENV_VARS}"
)
# Set sanitizer environment defaults (overridable by user env vars).
if(FUSILLI_ENABLE_ASAN)
set_tests_properties(${_RULE_NAME} PROPERTIES
ENVIRONMENT_MODIFICATION
"ASAN_OPTIONS=set_if_undefined:detect_leaks=1:halt_on_error=1"
"LSAN_OPTIONS=set_if_undefined:suppressions=${PROJECT_SOURCE_DIR}/build_tools/sanitizers/lsan_suppressions.txt"
)
endif()e6e57fe to
a95bc5c
Compare
Signed-off-by: Alexandra Sidorova <asidorov@amd.com>
| > - Debug builds (`-DCMAKE_BUILD_TYPE=Debug`) provide better stack traces. | ||
| > - Sanitizer builds have runtime overhead and are intended for development/testing, not production. | ||
| > - AddressSanitizer and Code Coverage cannot be used simultaneously, because both add heavy runtime instrumentation | ||
| and combining them often leads to slow/flaky tests and less reliable diagnostics. |
There was a problem hiding this comment.
nit: consider dropping the last bullet as this is likely not that prevalent of a use case
Implemented build support with AddressSanitizer for Unix systems with clang and added their builds in CI