Remove duplicate ASAN linker option. #4624
Conversation
tiledb/sm/array/array.cc
Outdated
| @@ -826,7 +826,7 @@ Status Array::reopen(uint64_t timestamp_start, uint64_t timestamp_end) { | |||
| array_uri_, | |||
| key->encryption_type(), | |||
| key->key().data(), | |||
| key->key().size(), | |||
| static_cast<uint32_t>(key->key().size()), | |||
There was a problem hiding this comment.
Introduced in #4601.
There was a problem hiding this comment.
The PR description is wholly inadequate. A couple of links are supporting references, not a description. What work was actually done? It doesn't look like anything was.
There's no indication that it even addresses the concerns from the PR, which is that the unit tests need to be built with ASAN. That means, and this time I need to be explicit, that we have some way of verifying that the unit tests are being built with ASAN. We have had problems in the past deluding ourselves that something was being tested that was not. As a specific instruction, ensure the following is done:
- Write two test related tests executables that compiles the same way that the unit tests do.
- One will be compiled both with and without ASAN
- The other will compiled only with ASAN
- These executables will terminate early when compiled with ASAN
- This is a death test, which Catch does not support, so in order not to fail CI there will need to be some kind of runner or wrapper around the executable.
- The executable that can be compiled without ASAN should succeed.
- Both executables use memory region poisoning.
- The executable that compiles both with and without ASAN uses the macro
ASAN_POISON_MEMORY_REGION, which has a feature test that omits the call when sanitization is absent. - The executable that compiles only with ASAN uses the function
__asan_poison_memory_region. This should fail to compile without ASAN; it would be good to test this failure in CMake, but that's optional.
- The executable that compiles both with and without ASAN uses the macro
- These executables should both run in CI alongside the unit tests.
The combination of these two executables ensures that we have neither false positives nor false negatives about ASAN compilation.
|
Can we move the greater ASAN validation work to a separate PR and just merge this one? For ease of review, the linker option is already being globally set here: TileDB/cmake/Modules/Sanitizer.cmake Line 73 in e11fe82 |
This comment was marked as outdated.
This comment was marked as outdated.
|
@teo-tsirpanis Let's update the title of this PR and the description to describe what you are trying to accomplish here. |
teo-tsirpanis
changed the title
|
@KiterLuc done. |
|
Closing for now. We don't have time to implement proper testing for this at the moment and we have tracking items for this in shortcut. |
This PR removes a linker option that is already globally specified here:
TileDB/cmake/Modules/Sanitizer.cmake
Line 73 in bb13be6
TYPE: BUILD
DESC: Remove duplicate ASAN linker option.