Page MenuHome

Build: disable address sanitizer for Cycles kernels with GCC
ClosedPublic

Authored by Brecht Van Lommel (brecht) on Aug 2 2019, 5:34 PM.

Details

Summary

It's extremely slow to compile and run, so just disable it. For Clang it's
still enabled since that appears to work ok.

This also limits the -fno-sanitize=vptr flag to the Cycles kernel with GCC
and Clang, since it was added specifically to work around an issue there.
This should solve the issue where this flag was used for MSVC as noted in D5149.

Diff Detail

Repository
rB Blender
Branch
dev-config (branched from master)
Build Status
Buildable 4266
Build 4266: arc lint + arc unit

Event Timeline

Sergey Sharybin (sergey) requested changes to this revision.Aug 2 2019, 5:58 PM

I don't think it's great idea to disable ASAN options for Debug builds.
I don't have issues compiling or running debug kernels with ASAN enabled. And that does help catching some mistakes and bugs.

For RelWithDebInfo and Release it is indeed nice to disable.

This revision now requires changes to proceed.Aug 2 2019, 5:58 PM
Brecht Van Lommel (brecht) updated this revision to Diff 16806.EditedAug 2 2019, 7:26 PM

Disable only for RelWithDebInfo builds (there is no asan in Release).

For me the Debug kernel.cpp compilation still is very slow though.

gcc 7.4.0: 1.5s
gcc 7.4.0 asan: 123s
clang 6.0.0: 0.3s
clang 6.0.0 asan: 1.3s

Hrm, weird. Can not say i remember such slow compile times, but sounds like it is too annoying for people.

On another hand, i don't like idea of not being able to run kernel with ASAN. Can i talk you into adding WITH_CYCLES_PARANOID_DEBUG option disabled by default and controlling ASAN for debug builds? :)

Disable kernel asan for GCC by default again, add option to enable

Is there any harm to remove if(WITH_COMPILER_ASAN) checks? From historical reasons i didn't use it and instead was passing -fsanitize via CMake's CFlags. If we remove the check the new code will "sanitize" compile times for such manually injected cflags as well?

No real problem I think, removed if(WITH_COMPILER_ASAN)

This revision is now accepted and ready to land.Aug 5 2019, 3:22 PM