Halide icon indicating copy to clipboard operation
Halide copied to clipboard

Add ASAN support to CMake via toolchain file

Open steven-johnson opened this issue 2 years ago • 14 comments

Perhaps a cleaner approach for ASAN in CMake, this uses a toolchain file to drive clang to run asan -- the point here being that everything participating in the ASAN Party has to use the same version of the same compiler (including the bitcode in our runtime), so this means we pretty much have to use the LLVM-provided clang.

Note that (as written) it will only work on Linux, though it could probably be adapted to work on any posixy system without much work. Since we'd probably only run sanitizer coverage on our Linux bots anyway, this is likely fine.

Note also that it requires building some C++ runtime code in LLVM that we currently don't do by default.

Note also that it currently doesn't test any Python code under ASAN; to make that work, we'd need to use -shared-asan (vs Clang's default of -static-asan); in theory that can be made to work, but it requires setting an LD_PRELOAD env var in lots of places across CMake, which is uglier than I'd like, so I have just skipped it for now. (Maybe there's a cleaner way?)

Anyway, if this approach works, we'd probably add more toolchain files for other sanitizers.

steven-johnson avatar Aug 04 '22 20:08 steven-johnson

Are those exact settings necessary

It didn't work at all until I added those, so, apparently :-)

steven-johnson avatar Aug 04 '22 22:08 steven-johnson

I have moved the sanitizer environment variables to presets for a few reasons:

  1. They were complicating our build rules considerably and necessitated hacks in custom commands.
  2. As environment variables, they can be set externally.
  3. The build itself succeeds without those variables set.
  4. We should probably know if the build starts to fail without those set.

Using the preset is easy and should be how we implement this in the buildbots:

$ cmake --preset linux-x64-asan -DLLVM_ROOT=/path/to/llvm
$ cmake --build --preset linux-x64-asan
$ ctest --preset linux-x64-asan

Where /path/to/llvm is the folder containing bin/clang++. This sequence of commands works for me on Google infra.

alexreinking avatar Aug 11 '22 01:08 alexreinking

LGTM

steven-johnson avatar Aug 11 '22 01:08 steven-johnson

With the newest commit, the apps have a corresponding preset. From the apps directory, run:

$ cmake --preset linux-x64-asan -DLLVM_ROOT=/path/to/llvm -DHalide_ROOT=/path/to/halide-install
$ cmake --build --preset linux-x64-asan
$ ctest --preset linux-x64-asan

I note that the apps require the ASAN options set during the build, too. This is also working for me on gLinux.

alexreinking avatar Aug 11 '22 02:08 alexreinking

$ cmake --build --preset linux-x64-asan

grumble, it really doesn't matter, but I am grumpy about not being able to just use ninja directly here

steven-johnson avatar Aug 11 '22 16:08 steven-johnson

$ cmake --preset linux-x64-asan -DLLVM_ROOT=/path/to/llvm

Why do we need to set LLVM_ROOT here? The buildbots have set only LLVM_DIR for ~ever.

steven-johnson avatar Aug 11 '22 16:08 steven-johnson

I am grumpy about not being able to just use ninja directly here

For compiling Halide, which doesn't require any env vars to be set, you can just call ninja. For the apps, you'd need to ASAN_OPTIONS some other way (have you ever used direnv?)

alexreinking avatar Aug 11 '22 16:08 alexreinking

(have you ever used direnv?)

Yikes, no, magic hidden things that set env vars based on my location is (to me) an indication of a system gone wrong

steven-johnson avatar Aug 11 '22 16:08 steven-johnson

Yikes, no, magic hidden things that set env vars based on my location is (to me) an indication of a system gone wrong

Eh, I like it for setting some common options locally, but to each their own.

alexreinking avatar Aug 11 '22 16:08 alexreinking

Why do we need to set LLVM_ROOT here? The buildbots have set only LLVM_DIR for ~ever.

To be more on point: it seems suboptimal to have to set two LLVM related config vars here (esp when one can generally be inferred from the other); since LLVM_DIR is already set, is there a compelling reason we can't assume LLVM_ROOT is something like ${LLVM_DIR}/../../.. ?

steven-johnson avatar Aug 11 '22 16:08 steven-johnson

cmake --build --preset linux-x64-asan

On gLinux, this fails for me with CMake Error: Could not read presets from /usr/local/google/home/srj/GitHub/Halide/build-asan: File not found

steven-johnson avatar Aug 11 '22 17:08 steven-johnson

is there a compelling reason we can't assume LLVM_ROOT is something like ${LLVM_DIR}/../../.. ?

On Ubuntu, there are two copies of LLVMConfig.cmake, one in LLVM_ROOT/cmake and another in LLVM_ROOT/lib/cmake/llvm. You don't need to set LLVM_DIR if you set LLVM_ROOT.

alexreinking avatar Aug 11 '22 17:08 alexreinking

On gLinux, this fails for me with CMake Error: Could not read presets from /usr/local/google/home/srj/GitHub/Halide/build-asan: File not found

Don't change directory, run all three commands from the root Halide dir.

alexreinking avatar Aug 11 '22 17:08 alexreinking

LGTM

steven-johnson avatar Aug 11 '22 19:08 steven-johnson

Windows failure is unrelated

alexreinking avatar Aug 11 '22 22:08 alexreinking