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


hebasto real-or-random fanquake

Labels
assurance build side-channel

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: 2024-12-21 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me