Add a cmake option to activate address sanitizers #146
Conversation
fmauch
force-pushed
the
address_sanitizer
branch
2 times, most recently
from
1c7d3f4
to
5277534
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #146 +/- ##
==========================================
- Coverage 70.62% 70.21% -0.42%
==========================================
Files 69 69
Lines 2526 2521 -5
Branches 321 321
==========================================
- Hits 1784 1770 -14
- Misses 562 567 +5
- Partials 180 184 +4
☔ View full report in Codecov by Sentry. |
CMakeLists.txt
Outdated
| if(WITH_ASAN) | ||
| add_compile_options(-fsanitize=address) | ||
| add_link_options(-fsanitize=address) | ||
| endif() |
There was a problem hiding this comment.
I am not a big fan of setting these flags globally, especially for libraries. I think
target_compile_options(urcl PRIVATE -fsanitize=address)
target_link_options(urcl PRIVATE -fsanitize=address)
would be the better choice.
There was a problem hiding this comment.
In principle I would agree. However, using this inside the main CMakeLists.txt
...
add_library(ur_client_library::urcl ALIAS urcl)
if(WITH_ASAN)
target_compile_options(urcl PRIVATE -fsanitize=address)
target_link_options(urcl PRIVATE -fsanitize=address)
endif()
target_compile_options(urcl PRIVATE -Wall -Wextra -Wno-unused-parameter)
target_compile_options(urcl PUBLIC ${CXX17_FLAG})
...leads to this (without further modifications)
build/examples/dashboard_example 192.168.56.101
==416292==ASan runtime does not come first in initial library list; you should either link runtime to your application or manually preload it with LD_PRELOAD.
So, one has to add the targeted sanitizer to each target, as the targets depending on urcl won't work (won't run) otherwise. Since from a maintenance perspective it seems not right to copy those four lines to each of our many targets I would suggest sticking with the global flag. It should only be activated manually for testing purposes, so I guess, this would be valid price to pay.
There was a problem hiding this comment.
Can you try using
target_compile_options(urcl PUBLIC -fsanitize=address)
target_link_options(urcl PUBLIC -fsanitize=address)
instead? From my understanding that should serve exactly this role, e.g. propagating to all targets linking to urcl.
There was a problem hiding this comment.
Sounds like a sane idea, will try that.
fmauch
force-pushed
the
address_sanitizer
branch
2 times, most recently
from
81a3157
to
277c323
Compare
We keep a pointer to the raw data buffer received from the comm interfaces in our packages. This was held as a unique_ptr<uint8_t> while we were storing a uint8_t type inside. This commit changes the pointer's type to the correct one.
test_tcp_server had a memory leak by not deleting manually allocated clients.
fmauch
force-pushed
the
address_sanitizer
branch
from
c3d260e
to
9f852b8
Compare
|
@RobertWilbrandt I've updated the PR using targeted compile options. This should now probably be fine for merging |
RobertWilbrandt
merged commit 9a59e81
into
UniversalRobots:master
In an effort to track down memory issues, I started activating address sanitizers on the urcl. I think, we should activate them by default inside CI in order to spot issues early on.