build: pass sanitize flags to instrument libsecp256k1 code #27991
Conversation
secp256k1 functions are now instrumented: ```asm objdump --disassemble-symbols=_secp256k1_xonly_pubkey_serialize src/test/fuzz/fuzz | grep sanitizer_cov 100eb5244: 73 5d 01 94 bl 0x100f0c810 <___sanitizer_cov_trace_const_cmp8> 100eb53f0: cc 5c 01 94 bl 0x100f0c720 <___sanitizer_cov_trace_pc_indir> 100eb5418: c2 5c 01 94 bl 0x100f0c720 <___sanitizer_cov_trace_pc_indir> 100eb5444: b7 5c 01 94 bl 0x100f0c720 <___sanitizer_cov_trace_pc_indir> ```
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Is it guaranteed that sanitizer flags for a C++ compiler are acceptible by a C compiler? |
|
Concept ACK All of the CI tasks that make use of the |
| @@ -1956,7 +1957,11 @@ CPPFLAGS_TEMP="$CPPFLAGS" | |||
| unset CPPFLAGS | |||
| CPPFLAGS="$CPPFLAGS_TEMP" | |||
|
|
|||
| if test "$use_sanitizers" != ""; then | |||
| ac_configure_args="${ac_configure_args} --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --disable-module-ecdh CFLAGS=${SANITIZER_CFLAGS}" | |||
There was a problem hiding this comment.
Won't this lose the other CFLAGS currently passed by the user?
|
Maybe mark as draft for as long as CI is red? |
|
FWIW, CMake works out of the box :) |
When does CI happen? Why would CI be red in this case? |
…pressions for `src/secp256k1` subtree a747774 Add UBSan `-fsanitize=integer` suppressions for `src/secp256k1` subtree (Hennadii Stepanov) Pull request description: Required for bitcoin/bitcoin#27991 (see the [comment](bitcoin/bitcoin#27991 (comment))) and for the upcoming CMake-based build system. ACKs for top commit: MarcoFalke: lgtm ACK a747774 Tree-SHA512: 602fa3ad22d3b0f6981a51358677d2347c92c4c9f59626b497af10f7ba828ede37227d8ee717f089bf33bde5efe0854d53acc89bea46f0955e62b7f22c454d05
|
Maybe we can followup with this in future (regardless of anything CMake related). |
… code cbea49c build: Pass sanitize flags to instrument `libsecp256k1` code (Hennadii Stepanov) Pull request description: This PR is a revived #27991 with an addressed [comment](#27991 (comment)). Fixes #27990. Might be tested as follows: ``` $ ./autogen.sh && ./configure --enable-fuzz --with-sanitizers=fuzzer CC=clang-13 CXX=clang++-13 $ make clean > /dev/null && make $ objdump --disassemble=secp256k1_xonly_pubkey_serialize src/test/fuzz/fuzz | grep __sanitizer_cov 1953bd0:e8 bb c6 05 ff call 9b0290 <__sanitizer_cov_trace_const_cmp8> 1953d32:e8 69 c4 05 ff call 9b01a0 <__sanitizer_cov_trace_pc_indir> 1953d58:e8 43 c4 05 ff call 9b01a0 <__sanitizer_cov_trace_pc_indir> 1953d82:e8 19 c4 05 ff call 9b01a0 <__sanitizer_cov_trace_pc_indir> ``` ACKs for top commit: fanquake: ACK cbea49c dergoegge: reACK cbea49c Tree-SHA512: 801994e75b711d20eaf0d675f378da07d693f4a7de026efd93860f5f1deabed855a83eca3561725263e4fe605fcc5f91eb73c021ec91c831864e6deb575e3885
secp256k1 functions are now instrumented:
Not sure if this is the best solution. Would fix #27990.