-
Notifications
You must be signed in to change notification settings - Fork 35.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
build: Enable fuzz binary in MSVC #29774
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. |
Friendly ping @sipa @maflcko @sipsorcery @dergoegge ;) |
@hebasto What is the issue with the bitdeque and miniscript fuzz tests? |
Here is a branch with included
Here is a branch with included
|
|
utACK 239d1d6 Nice, I'm surprised how few changes/disabling of tests were needed to make this work. |
I get 14 compiler errors when building this PR with Visual Studio. One example on line 282 of transaction_tests.cpp:
Gives compiler error of:
It's easy enough to fix by casting the unsigned char[] to std::string but perhaps there's a deeper issue? |
How does your setup differ from the one in the CI? |
I believe this is unrelated to this PR, see #29051. |
I'm on Windows and using msvc but I did do a clean build but it didn't help. I suspect it's down to msvc being less forgiving than gcc, probably due to a recent msvc change although I couldn't find anything in the release notes. Apolgies for dumping a screenshot but it does show a succinct summary of the issue. This is from an attempt to compile (no linking involved) the single blockfilter_tests.cpp file. |
What VS / MSVC versions do you use? |
Does it compile on the master branch for you? |
Visual Studio (including msvc) updates get automatically pushed every couple of weeks. I'm generally always pretty close to the latest stable version. Currently Visual Studio 2022 17.8.0
|
No. I only checked that after posting the compiler error message. The msvc compiler error isn't related to this PR, it's on master as well. |
Could you please open a separate issue then? FWIW, I have no compile errors for the master branch @ b5d2118 using VS Community 2022 17.9.5. For example, the
|
239d1d6
to
277b155
Compare
The branch has been reworked to address @maflcko's comments.
Whether |
47cedee fuzz: Introduce `BITCOINFUZZ` environment variable (Hennadii Stepanov) 1573e9a fuzz, refactor: Deduplicate fuzz binary path creation (Hennadii Stepanov) Pull request description: These changes are split from #29774 and can be beneficial on their own. The new `BITCOINFUZZ` environment variable complements the already existing set of variables used by tests: https://github.com/bitcoin/bitcoin/blob/b5d21182e5a66110ce2796c2c99da39c8ebf0d72/test/functional/test_framework/test_framework.py#L238-L243 ACKs for top commit: maflcko: lgtm ACK 47cedee davidgumberg: utACK 47cedee Tree-SHA512: 45809cfd13dc4a45c44cc433184352e84726cb95bea80fd8f581c59a0b8b0a5495260ff66922f9c57c38adbdbdd102439238f370fd49d6ea27a241a5e6249895
56e1e5d refactor, bench, fuzz: Drop unneeded `UCharCast` calls (Hennadii Stepanov) Pull request description: The `CKey::Set()` template function handles `std::byte` just fine: https://github.com/bitcoin/bitcoin/blob/b5d21182e5a66110ce2796c2c99da39c8ebf0d72/src/key.h#L105 Noticed in #29774 (comment). ACKs for top commit: maflcko: lgtm ACK 56e1e5d laanwj: Seems fine, code review ACK 56e1e5d hernanmarino: ACK 56e1e5d Tree-SHA512: 0f6b6e66692e70e083c7768aa4859c7db11aa034f555d19df0e5d33b18c0367ba1c886bcb6be3fdea78248a3cf8285576120812da55b995ef5e6c94a9dbd9f7c
See: - https://learn.microsoft.com/en-us/cpp/build/reference/zc-preprocessor - https://learn.microsoft.com/en-us/cpp/preprocessor/preprocessor-experimental-overview Otherwise, the "traditional" MSVC preprocessor fails to parse the `FUZZ_TARGET` and `DETAIL_FUZZ` macros because of behavior changes highlighted in the docs mentioned above.
37cda55
to
18fd522
Compare
Addressed the recent @maflcko's comments. |
Haven't reviewed anything in build_msvc lgtm ACK 18fd522 🔍 Show signatureSignature:
|
Mind having another look into this PR? |
tACK 09f5a74. |
Wrong commit hash? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 18fd522
The #29983 resolves the issue with the former. |
774359b build, msvc: Compile `test\fuzz\bitdeque.cpp` (Hennadii Stepanov) 85f50a4 refactor: Fix "error C2248: cannot access private member" on MSVC (Hennadii Stepanov) Pull request description: This PR resolves one point from the #29774 (comment): > What is the issue with the bitdeque... ? ACKs for top commit: maflcko: lgtm ACK 774359b sipa: utACK 774359b achow101: ACK 774359b dergoegge: utACK 774359b Tree-SHA512: dba5c0217b915468af08475795437a10d8e8dedfadeb319f36d9b1bf54a91a8b2c61470a6047565855276c2bc8589c7776dc19237610b65b57cc841a303de8b3
Ported to the CMake-based build system in hebasto#182. |
fc91bfe fixup! cmake: Add fuzzing options (Hennadii Stepanov) 5927338 fixup! cmake: Add platform-specific definitions and properties (Hennadii Stepanov) dcf2e66 refactor: Make 64-bit shift explicit (Hennadii Stepanov) a7edea8 refactor: Fix "error C2248: cannot access private member" on MSVC (Hennadii Stepanov) Pull request description: This PR ports bitcoin#29774 after the recent sync/rebase [PR](#179). Also included changes from bitcoin#29983. Top commit has no ACKs. Tree-SHA512: 562919ec9b89401d193c6d59a8634f5086a1a063b6ee075c465c5dbc9bf59da4bb1eb7b826deac91f470e22fe9bf049b43b7106bdc66c5d26c715d68a1008f4d
9155b73 build, msvc: Compile test\fuzz\miniscript.cpp (Hennadii Stepanov) Pull request description: This PR resolves the remained point from the #29774 (comment): > What is the issue with the ... miniscript fuzz tests? From the CI [log](https://github.com/bitcoin/bitcoin/actions/runs/8941546183/job/24562123707?pr=30031#step:29:234): ``` miniscript_script: succeeded against 721 files in 1s. Run miniscript_script with args ['D:\\a\\bitcoin\\bitcoin\\src\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/miniscript_script')] miniscript_smart: succeeded against 1429 files in 2s. Run miniscript_smart with args ['D:\\a\\bitcoin\\bitcoin\\src\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/miniscript_smart')] miniscript_stable: succeeded against 1871 files in 2s. Run miniscript_stable with args ['D:\\a\\bitcoin\\bitcoin\\src\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/miniscript_stable')] miniscript_string: succeeded against 918 files in 3s. Run miniscript_string with args ['D:\\a\\bitcoin\\bitcoin\\src\\fuzz.exe', WindowsPath('D:/a/_temp/qa-assets/fuzz_seed_corpus/miniscript_string')] ``` ACKs for top commit: maflcko: ACK 9155b73 TheCharlatan: ACK 9155b73 Tree-SHA512: 967f199aac41733265532518ff7b1d881ba5a7bbde9f827d6a0b6d984c94a65b20d5854ce0ea247158eaa17b21d4c9f7d25c79bac17960788bacd2586112630b
Closes #29760.
Suggested in #29758 (comment).