search

osrf / osrf_testing_tools_cpp

Common testing tools for C++ which are used for testing in various OSRF projects.
Apache License 2.0
33 stars 29 forks source link

Add option, exported property, and C++ API for memory_tools. #28

Closed nuclearsandwich closed 5 years ago

nuclearsandwich commented 5 years ago

This adds a CMake option to explicitly disable memory tools, say to use ASan instead. It also adds an exported CMake property to describe the availability of memory tools.

This is intended to resolve #23, replaces #24 and is an alternative to #25. All in support of https://github.com/ros2/ros2/issues/665

nuclearsandwich commented 5 years ago

This is ready for design review, in particular I have the following questions:

How to detect LD_PRELOAD usage for is_installed()

The current draft just uses an ifdef based on whether memory_tools was built with the preload option enabled. Other checks we could do include checking the actual environment for LD_PRELOAD, or possibly grabbing a pointer to malloc and checking if it matches the memory_tools one. But I'm not sure if either approach is reasonable. Both seem more accurate than compile time config. It also occurs to me that we might be able to push is_installed() into the implementation-specific code which might give us more options for how to implement it.

Is a separate CMake property really needed?

I was trying to make it easier for downstream to distinguish between memory_tools being available but not used versus not available, therefore not used. But I suppose we could set the existing properties to indicate that same information. I gently prefer the CMake property because those are documented by CMake tooling versus out-of-band documentation for any environment variable conventions.

Breaking API or breaking docs for is_working

The documentation for is_working states that it returns true of hooks are installed and working. But in some cases (such as on aarch64) it also returns true if hooks aren't installed. This PR changes that so that the documentation is correct. But this will cause downstream test failures.

We could add a separate is_working_or_unavailable() function that handles the current use case and update is_working to match the docs, or we could add an is_installed_and_working() function to enforce the installed requirement and update the docs for the existing is_working to match the semantics of is_working_or_unavailable.

nuclearsandwich commented 5 years ago

Other things open for discussion / bikeshedding:

I'm not yet attached to any of the option, property, or function names introduced in this PR. Earlier work in #24. Rather than a negative option that defaults to off, I opted for a positive option that defaults to on (i.e. ENABLE_MEMORY_TOOLS_PRELOAD instead of DISABLE_MEMORY_TOOLS_PRELOAD). I did this because I think positive options scan more easily but if others feel differently I can invert it.

wjwwood commented 5 years ago

How to detect LD_PRELOAD usage for is_installed()

Currently, is_working indirectly checks this by seeing if it can catch an explicit memory allocation. However, if it fails to do so, it only means that a lack of LD_PRELOAD may be the cause, but there might be other causes.

So for me the question is whether or not you need to know if it's not working and it's specifically because of the missing preload.

Is a separate CMake property really needed?

I'm not sure, but it seems useful to me. It gives more flexibility to users of this package to decide how to write their tests, i.e. when to enable/disable/skip them.

Breaking API or breaking docs for is_working

Currently I don't see why breaking the API behavior for is_working would be necessary.

The documentation for is_working states that it returns true of hooks are installed and working. But in some cases (such as on aarch64) it also returns true if hooks aren't installed.

That should not be the case, it should only return true if it was capable of detecting a malloc using the hooks:

https://github.com/osrf/osrf_testing_tools_cpp/blob/399a25c5e47bb63118435777191f83599ed3500c/osrf_testing_tools_cpp/src/memory_tools/is_working.cpp#L43-L52

There's no logic that I'm aware of that handle special cases for things like aarch64. Are you talking about a different API (maybe a cmake one)?

nuclearsandwich commented 5 years ago

Thanks for the review so far. I think I must've had an unclean local build as I could have sworn I could demonstrate is_working() returning true when hooks weren't installed but I can no longer reproduce that behavior.

So for me the question is whether or not you need to know if it's not working and it's specifically because of the missing preload.

Now that I'm no longer able to reproduce any inconsistency between the documented function is_working and its implementation, I don't have any reason to need an explicit is_installed from C++. Consumer packages that have exclusive memory_tools tests can use the CMake API to skip them based on the newly exposed property and tests which use memory_tools during other tests can use is_working without asserting on it to determine whether or not it's running.

That reduces the scope of this PR to introducing the CMake option specific to this repository to force memory_tools not to set an LD_PRELOAD library and exporting the CMake property that consumer packages can use to check whether memory tools is available rather than duplicating the same checks (for aarch64) in all tests.

nuclearsandwich commented 5 years ago

CI with default behavior to check for regressions:

nuclearsandwich commented 5 years ago

The test failures on the Windows build are also present on the debug nightly https://ci.ros2.org/view/nightly/job/nightly_win_deb/1196/ I believe they are unrelated.

CI with -DENABLE_MEMORY_TOOLS_PRELOAD=OFF

nuclearsandwich commented 5 years ago

CI with latest changes, mostly to make sure I changed all the variable names to the same thing.