cmake: Disable eager MSan in ctime_tests #1532

pull hebasto wants to merge 4 commits into bitcoin-core:master from hebasto:240526-msan-cmake changing 4 files +45 −5
  1. hebasto commented at 2:56 PM on May 26, 2024: member

    Same as #1517, but for the CMakle build system.

    The second commit improves the configure summary (similar to https://github.com/hebasto/bitcoin/pull/189.

  2. real-or-random commented at 12:25 PM on May 27, 2024: contributor

    Concept ACK

  3. in cmake/CheckMemorySanitizer.cmake:9 in 7e2afc4968 outdated 
       0 | @@ -0,0 +1,16 @@
   1 | +include_guard(GLOBAL)
   2 | +include(CheckCSourceCompiles)
   3 | +
   4 | +function(check_memory_sanitizer output)
   5 | +  set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
   6 | +  check_c_source_compiles("
   7 | +    #if defined(__has_feature)
   8 | +    #  if __has_feature(memory_sanitizer)
   9 | +    //   MemorySanitizer is enabled.
    real-or-random commented at 12:27 PM on May 27, 2024:

    nit: We should probably use the same test program on configure and CMake (and perhaps avoid // comments which are invalid in C89, though that really is more of a philosophical concern.)

    hebasto commented at 12:45 PM on May 27, 2024:

    nit: We should probably use the same test program on configure and CMake

    I'd prefer to align Autotools' SECP_MSAN_CHECK code with CMake's check_memory_sanitizer to keep the latter straightforward without negated values.

    ... and perhaps avoid // comments which are invalid in C89

    Thanks. Fixed.

    real-or-random commented at 8:48 PM on May 27, 2024:

    I'd prefer to align Autotools' SECP_MSAN_CHECK code with CMake's check_memory_sanitizer to keep the latter straightforward without negated values.

    Fine with that if you want to add a commit that makes them consistent, but then we'll also need to #error if !defined(__has_feature).

    hebasto commented at 8:49 AM on May 28, 2024:

    I'd prefer to align Autotools' SECP_MSAN_CHECK code with CMake's check_memory_sanitizer to keep the latter straightforward without negated values.

    Fine with that if you want to add a commit that makes them consistent, but then we'll also need to #error if !defined(__has_feature).

    Thanks! Fixed.

  4. in CMakeLists.txt:267 in 7e2afc4968 outdated 
     262 | @@ -263,6 +263,17 @@ endif()
 263 |  
 264 |  set(CMAKE_C_VISIBILITY_PRESET hidden)
 265 |  
 266 | +set(print_msan_notice)
 267 | +if(SECP256K1_BUILD_CTIME_TESTS AND CMAKE_C_COMPILER_ID MATCHES "^(Clang|AppleClang)$")
    fanquake commented at 12:29 PM on May 27, 2024:

    Does Apple Clang makes sense here?

    fanquake commented at 12:34 PM on May 27, 2024:

    Actually, why are you hard coding compiler IDs at all? This just makes things less flexible, and the check should be enough.

    real-or-random commented at 12:38 PM on May 27, 2024:

    Yeah, I had the same thought. It saves a test compilation for GCC, but it's probably simpler to drop the check for clang. In #1517, I also added a similar check (but for "GCC or Clang", so it's much more useless). Let's just drop it.

    hebasto commented at 12:47 PM on May 27, 2024:

    Actually, why are you hard coding compiler IDs at all? This just makes things less flexible, and the check should be enough.

    Thanks! The compiler ID check has been deleted.

    real-or-random commented at 8:49 PM on May 27, 2024:

    Thanks, do you want to add a commit that also removes the $GCC check in configure.ac, just to keep the system consistent?

    hebasto commented at 8:50 AM on May 28, 2024:

    Thanks, do you want to add a commit that also removes the $GCC check in configure.ac, just to keep the system consistent?

    Sure. Added.

  5. real-or-random added the label assurance on May 27, 2024
  6. real-or-random added the label build on May 27, 2024
  7. real-or-random added the label side-channel on May 27, 2024
  8. hebasto force-pushed on May 27, 2024
  9. hebasto commented at 12:43 PM on May 27, 2024: member

    Addressed comments:

    avoid // comments which are invalid in C89

    Actually, why are you hard coding compiler IDs at all? This just makes things less flexible, and the check should be enough.

  10. cmake: Disable `ctime_tests` if build with `-fsanitize=memory`
    Clang >= 16 has `-fsanitize-memory-param-retval` turned on by default,
which is incompatible with
    7abf979a43
  11. cmake: Report more compiler details in summary abde59f52d
  12. autotools: Align MSan checking code with CMake's implementation 396e885886
  13. autotools: Delete unneeded compiler test
    This change makes both Autotools and CMake build systems consistent.
    f55703ba49
  14. hebasto force-pushed on May 28, 2024
  15. hebasto commented at 8:49 AM on May 28, 2024: member

    Addressed comments:

    ... to add a commit that makes them consistent, but then we'll also need to #error if !defined(__has_feature).

    ... to add a commit that also removes the $GCC check in configure.ac, just to keep the system consistent

  16. real-or-random approved
  17. real-or-random commented at 12:33 PM on June 5, 2024: contributor

    ACK f55703ba49454fc46226f4846fe292d4a3dfa3ef

  18. real-or-random merged this on Jun 10, 2024
  19. real-or-random closed this on Jun 10, 2024
  20. hebasto deleted the branch on Jun 10, 2024
Contributors
hebastoreal-or-randomfanquake
Labels
assurancebuildside-channel
Linked (view graph)
#1517 autotools: Disable eager MSan in ctime_tests
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-22 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me