Remove address sanitizer from project build

[rotu]

1. Don't automatically enable asan on Debug builds.
   2. Remove asan logic in the library build process.
2. Retain existing asan behavior in the CI builds.

The correct way to hook up address sanitizer depends on the eventual application using it and whether CycloneDDS is statically or dynamically linked.
Doing it improperly results in runtime crashes like:

    [ERROR] [1582664129.049907842] [rcl]: Error getting RMW implementation identifier / RMW implementation not installed (expected identifier of 'rmw_cyclonedds_cpp'), with error message 'failed to load shared library of rmw implementation. Exception: Cannot load library: /opt/ros/master/install/lib/libddsc.so.0: undefined symbol: __asan_option_detect_stack_use_after_return, at /opt/ros/master/src/ros2/rmw_implementation/rmw_implementation/src/functions.cpp:69', exiting with 1.
  >>>

Signed-off-by: Dan Rose dan@digilabs.io

[eboasson]

@rotu I understand the problem and I am ok with just not enabling address sanitizer by default in a debug build. What I don't understand is why you propose removing it from CMakeLists.txt altogether: I find it rather convenient that I can switch between sanitizers (or none at all) by just doing cmake -DUSE_SANITIZER=X and not have to override the compiler flags by hand all the time. I would imagine that simply changing the default to "none" is enough to solve your problem.

[rotu]

The reason I think it doesn’t belong in project build files is because those options must be chosen with the consuming app in mind. If you always want your debug builds to have asan, it might make sense to add those flags to CMAKE_C_FLAGS_DEBUG so your asan builds are even more convenient!

If you still feel this belongs here, I will restore that project build option.

[eboasson]

@rotu adding it to CMAKE_C_FLAGS_DEBUG sounds like it might be a reasonable middle way, but https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html suggests to me that it is a CMake variable and so it isn't clear to me how one would conveniently set while avoiding the original problem. (This statement may merely be proof of my not knowing CMake well.)

If instead if it is an environment variable, then simply setting it globally would mess up everything else. So that doesn't seem like such a good idea either. An environment variable like CYCLONEDDS_USE_SANITIZER could perhaps be an option.

How about you split this PR into one that just changes the default behaviour to not automatically enable address sanitizer, and another one proposing a change to the way address sanitizer is enabled in the first place? Then I can already merge the former while having some more time to decide on the second, or get input from other people with more knowledge of CMake than I have. (Any ideas, @k0ekk0ek?)

[k0ekk0ek]

I actually intended for some comments but didn't get around to it yet. I think we may want to have a look at CMAKE_CONFIGURATION_TYPES. Next to Debug we could then have a Sanitizer configuration(?) We'd have to test how well it works, but that seems like a clean solution. Of course, using the current USE_SANITIZER option just works(?) Instead of on by default, leave it of by default and add flags to CMAKE_C_FLAGS_DEBUG if it's enabled. The latter seems like a very pragmatic way...

[rotu]

Alright. I rolled back the scope of this PR. I still don't know the best way forward, but I think this is good enough for now

[eboasson]

Thanks @rotu, not merely for rolling back but also improving in the process. I'm certain this is a step in the right direction, and at least it solves the problems caused by enabling the address by default.

I was wondering whether perhaps this one of the things toolchain files are for ...

[rotu]

It's a great use for toolchain files. It's also a good use for Colcon's defaults.yaml. Or for creating a direnv script to set CFLAGS / CXXFLAGS / LDFLAGS.