open-rmf / rmf_traffic

Traffic management libraries for RMF
Apache License 2.0
28 stars 26 forks source link

Add address sanitizer. #27

Closed arjo129 closed 3 years ago

arjo129 commented 3 years ago

Signed-off-by: Arjo Chakravarty arjo@openrobotics.org

New feature implementation

Implemented feature

Adding address sanitizer as part of open-rmf/rmf#47 .

Implementation description

The structure is essentially the same as pr #20. Basically ASAN also has a colcon mixin and we can reuse the stuff from ros-tooling. The good news is that there are no memory leaks so far while running the tests.

codecov[bot] commented 3 years ago

Codecov Report

Merging #27 (6273d53) into main (f93fc72) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #27   +/-   ##
=======================================
  Coverage   32.66%   32.66%           
=======================================
  Files         155      155           
  Lines       15633    15633           
  Branches    10435    10435           
=======================================
  Hits         5107     5107           
  Misses       1694     1694           
  Partials     8832     8832           
Flag Coverage Δ
tests 32.66% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

arjo129 commented 3 years ago

Usually, if there are errors ASAN will cry out very loudly on the STDERR interface and return non zero execution value thus triggering the :x: on the CI. One can check the log in the workflows tab for the exact details of the memory leak. One thing to note, is ASAN is only good for memory leaks which hopefully are few given the use of RAII and smart pointers. For uninitialized memory usage we could explore msan, however using msan is a huge headache as all system libraries need to be recompiled with msan enabled. An alternative would be to use valgrind's memcheck which I don't believe has such limitations (although it has much more overhead),

gbiggs commented 3 years ago

Usually, if there are errors ASAN will cry out very loudly on the STDERR interface and return non zero execution value thus triggering the x on the CI.

How easy are those logs to find and access for offline reading?

An alternative would be to use valgrind's memcheck which I don't believe has such limitations (although it has much more overhead),

Ideally we only want memory errors in our code or our usage of other APIs (e.g. ROS APIs). Valgrind is possibly easier to restrict to that than msan?

I think the overhead of valgrind is acceptable since we will likely only run this test once a day in the nightly build.

arjo129 commented 3 years ago

How easy are those logs to find and access for offline reading?

The [following instructions]() allow you to download the logs ``. To be honest, the moment asan detects one leak it will fail, so its a bit like python it that sense.

Ideally we only want memory errors in our code or our usage of other APIs (e.g. ROS APIs). Valgrind is possibly easier to restrict to that than msan?

I don't know if we can restrict it. But at least I don't need to instrument every dependency like I do with msan (msan will give false positives otherwise). I'll get cracking on valgrind.

gbiggs commented 3 years ago

Take a look at this comment for a nice way to handle the colcon output.