build: Enable fuzz binary in MSVC #29774

hebasto · 2024-03-31T16:00:12Z

Closes #29760.

Suggested in #29758 (comment).

DrahtBot · 2024-03-31T16:00:15Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, sipsorcery, sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29868 (Reintroduce external signer support for Windows by hebasto)
#29494 (build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes by maflcko)
#28526 (Add 1-way SSE4 SHA256 implementation using intrinsics for MSVC builds by hebasto)

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.

hebasto · 2024-03-31T16:01:02Z

Friendly ping @sipa @maflcko @sipsorcery @dergoegge ;)

sipa · 2024-03-31T16:07:49Z

@hebasto What is the issue with the bitdeque and miniscript fuzz tests?

hebasto · 2024-03-31T16:44:57Z

What is the issue with the bitdeque and miniscript fuzz tests?

Here is a branch with included bitdeque.cpp and the excerpt from its build log:

  bitdeque.cpp
D:\a\bitcoin\bitcoin\src\util\bitdeque.h(91,20): error C2248: 'bitdeque<128>::BITS_PER_WORD': cannot access private member declared in class 'bitdeque<128>' [D:\a\bitcoin\bitcoin\build_msvc\fuzz\fuzz.vcxproj]
  D:\a\bitcoin\bitcoin\src\util\bitdeque.h(28,26): note: see declaration of 'bitdeque<128>::BITS_PER_WORD'
  D:\a\bitcoin\bitcoin\src\test\fuzz\bitdeque.cpp(18,23): note: see declaration of 'bitdeque<128>'
  D:\a\bitcoin\bitcoin\src\util\bitdeque.h(91,20): note: the template instantiation context (the oldest one first) is
  D:\a\bitcoin\bitcoin\src\test\fuzz\bitdeque.cpp(215,17): note: see reference to class template instantiation 'bitdeque<128>::Iterator<true>' being compiled
  D:\a\bitcoin\bitcoin\src\util\bitdeque.h(89,32): note: while compiling class template member function 'bitdeque<128>::Iterator<true>::difference_type operator -(const bitdeque<128>::Iterator<true> &,const bitdeque<128>::Iterator<true> &)'
  D:\a\bitcoin\bitcoin\src\test\fuzz\bitdeque.cpp(215,17): note: see the first reference to 'operator -' in 'bitdeque_fuzz_target::<lambda_18>::operator ()'
D:\a\bitcoin\bitcoin\src\util\bitdeque.h(91,20): error C2248: 'bitdeque<128>::BITS_PER_WORD': cannot access private member declared in class 'bitdeque<128>' [D:\a\bitcoin\bitcoin\build_msvc\fuzz\fuzz.vcxproj]
  (compiling source file '../../src/test/fuzz/bitdeque.cpp')
  D:\a\bitcoin\bitcoin\src\util\bitdeque.h(28,26):
  see declaration of 'bitdeque<128>::BITS_PER_WORD'
  D:\a\bitcoin\bitcoin\src\test\fuzz\bitdeque.cpp(18,23):
  see declaration of 'bitdeque<128>'
  D:\a\bitcoin\bitcoin\src\util\bitdeque.h(91,20):
  the template instantiation context (the oldest one first) is
  	D:\a\bitcoin\bitcoin\src\test\fuzz\bitdeque.cpp(215,17):
  	see reference to class template instantiation 'bitdeque<128>::Iterator<true>' being compiled
  	D:\a\bitcoin\bitcoin\src\util\bitdeque.h(89,32):
  	while compiling class template member function 'bitdeque<128>::Iterator<true>::difference_type operator -(const bitdeque<128>::Iterator<true> &,const bitdeque<128>::Iterator<true> &)'
  		D:\a\bitcoin\bitcoin\src\test\fuzz\bitdeque.cpp(215,17):
  		see the first reference to 'operator -' in 'bitdeque_fuzz_target::<lambda_18>::operator ()'

Here is a branch with included miniscript.cpp and the excerpt from its build log:

  miniscript.cpp
D:\a\bitcoin\bitcoin\src\script\miniscript.h(154,13): error C2248: 'miniscript::Type::Type': cannot access private member declared in class 'miniscript::Type' [D:\a\bitcoin\bitcoin\build_msvc\fuzz\fuzz.vcxproj]
  D:\a\bitcoin\bitcoin\src\script\miniscript.h(127,5): note: see declaration of 'miniscript::Type::Type'
  D:\a\bitcoin\bitcoin\src\script\miniscript.h(122,7): note: see declaration of 'miniscript::Type'
  D:\a\bitcoin\bitcoin\src\test\fuzz\miniscript.cpp(1226,53): note: while evaluating constexpr function 'miniscript::operator ""_mst'

Richlilyjoy100 · 2024-03-31T20:30:55Z

Closes #29760.

Suggested in #29758 (comment).

Once this PR collects concept ACKs, I'm going to open dedicated PRs to fix portability issues in the code.

src/test/fuzz/util/descriptor.cpp

sipa · 2024-04-01T12:43:17Z

utACK 239d1d6

Nice, I'm surprised how few changes/disabling of tests were needed to make this work.

src/test/fuzz/util/descriptor.cpp

src/test/fuzz/fuzz.cpp

.github/workflows/ci.yml

sipsorcery · 2024-04-01T17:27:51Z

I get 14 compiler errors when building this PR with Visual Studio.

One example on line 282 of transaction_tests.cpp:

UniValue tests = read_json(json_tests::tx_invalid);

Gives compiler error of:

Error	C2664	'UniValue read_json(const std::string &)': 
cannot convert argument 1 from 'const unsigned char [53413]' to 'const std::string &'	
test_bitcoin	C:\dev\github\bitcoin\src\test\transaction_tests.cpp	282

It's easy enough to fix by casting the unsigned char[] to std::string but perhaps there's a deeper issue?

hebasto · 2024-04-01T18:08:22Z

@sipsorcery

I get 14 compiler errors when building this PR with Visual Studio.

How does your setup differ from the one in the CI?

dergoegge · 2024-04-05T14:21:01Z

One example on line 282 of transaction_tests.cpp:

I believe this is unrelated to this PR, see #29051. make clean and rebuilding should fix it.

sipsorcery · 2024-04-05T21:38:03Z

I believe this is unrelated to this PR, see #29051. make clean and rebuilding should fix it.

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.

hebasto · 2024-04-06T12:20:25Z

@sipsorcery

I'm on Windows and using msvc but I did do a clean build but it didn't help.

What VS / MSVC versions do you use?

hebasto · 2024-04-06T12:22:26Z

This is from an attempt to compile (no linking involved) the single blockfilter_tests.cpp file.

Does it compile on the master branch for you?

sipsorcery · 2024-04-06T12:24:17Z

@sipsorcery

I'm on Windows and using msvc but I did do a clean build but it didn't help.

What VS / MSVC versions do you use?

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

msbuild --version
MSBuild version 17.8.3+195e7f5a3 for .NET Framework
17.8.3.51904

sipsorcery · 2024-04-06T12:25:46Z

This is from an attempt to compile (no linking involved) the single blockfilter_tests.cpp file.

Does it compile on the master branch for you?

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.

hebasto · 2024-04-06T13:46:29Z

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 test_bitcoin.exe compiles for both Debug and Release configurations:

> msbuild --version
MSBuild version 17.9.8+b34f75857 for .NET Framework
17.9.8.16306
> py -3 build_msvc\msvc-autogen.py
> msbuild build_msvc\bitcoin.sln -p:UseMultiToolTask=true -p:Configuration=Debug -maxCpuCount -v:minimal -noLogo -t:test_bitcoin
> msbuild build_msvc\bitcoin.sln -p:UseMultiToolTask=true -p:Configuration=Release -maxCpuCount -v:minimal -noLogo -t:test_bitcoin

hebasto · 2024-04-06T16:53:17Z

The branch has been reworked to address @maflcko's comments.

... but bool return value + exceptions doesn't make sense, does it?

Whether std::istream::read() throws or not depends on how exceptions() is set. The suggested code does not use exceptions.

src/test/fuzz/multiplication_overflow.cpp

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

maflcko · 2024-04-18T09:22:58Z

I believe this is unrelated to this PR, see #29051. make clean and rebuilding should fix it.

I'm on Windows and using msvc but I did do a clean build but it didn't help.

Does it still happen after you clear the git repo carefully and start from scratch? git clean -dffx, or similar, but this will delete all non-git files, so be careful.

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.

See https://docs.python.org/3/library/subprocess.html

hebasto · 2024-04-18T09:28:57Z

Addressed the recent @maflcko's comments.

maflcko · 2024-04-18T09:36:13Z

Haven't reviewed anything in build_msvc

lgtm ACK 18fd522 🔍

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 18fd522ca9a74cf8690a6c9b9b729e78c6ed41fd 🔍
gNifTt5s+7OtYIlaXMdEJ3HLZ9C6g8ytDavFNfVc383QbognEyerTmREnewCCAQL5VnglQrDEpSetYJvM+yaAg==

hebasto · 2024-04-18T10:30:27Z

@dergoegge @sipa @sipsorcery

Mind having another look into this PR?

sipsorcery · 2024-04-26T20:12:39Z

tACK 09f5a74.

hebasto · 2024-04-26T20:17:57Z

@sipsorcery

tACK 09f5a74.

Wrong commit hash?

:)

sipsorcery · 2024-04-26T20:21:57Z

@sipsorcery

tACK 09f5a74.

Wrong commit hash?

:)

Every time :( I wish there were timestamps on the Github list. If only my old brain could remember to use git log next time.

tACK 18fd522

sipa

utACK 18fd522

hebasto · 2024-04-28T07:03:25Z

What is the issue with the bitdeque and miniscript fuzz tests?

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

hebasto · 2024-05-01T14:25:24Z

Ported to the CMake-based build system in hebasto#182.

hebasto · 2024-05-03T12:51:33Z

@sipa

What is the issue with the ... miniscript fuzz tests?

The miniscript issue is resolved with your #28657 :)

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

DrahtBot added the Build system label Mar 31, 2024

Empact reviewed Mar 31, 2024

View reviewed changes

src/test/fuzz/util/descriptor.cpp Outdated Show resolved Hide resolved

This was referenced Apr 1, 2024

build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes #29494

Merged

Add 1-way SSE4 SHA256 implementation using intrinsics for MSVC builds #28526

Closed

Enable HW-accelerated implementations of SHA256 for MSVC builds #24773

Closed

maflcko reviewed Apr 1, 2024

View reviewed changes

src/test/fuzz/util/descriptor.cpp Outdated Show resolved Hide resolved

maflcko reviewed Apr 1, 2024

View reviewed changes

src/test/fuzz/fuzz.cpp Outdated Show resolved Hide resolved

src/test/fuzz/fuzz.cpp Outdated Show resolved Hide resolved

src/test/fuzz/fuzz.cpp Outdated Show resolved Hide resolved

.github/workflows/ci.yml Outdated Show resolved Hide resolved

This was referenced Apr 6, 2024

refactor, bench, fuzz: Drop unneeded UCharCast calls #29820

Merged

fuzz: Some test/fuzz/test_runner.py improvements #29821

Merged

hebasto force-pushed the 240331-msvc-fuzz branch from 239d1d6 to 277b155 Compare April 6, 2024 16:46

fanquake reviewed Apr 6, 2024

View reviewed changes

src/test/fuzz/multiplication_overflow.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 9, 2024

[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP #29790

Draft

hebasto added 5 commits April 18, 2024 10:27

build, msvc: Build fuzz.exe binary

19dcedd

ci, msvc: Add "Clone fuzz corpus" step

23cb820

fuzz: Pass SystemRoot environment variable to subprocess

52933d7

See https://docs.python.org/3/library/subprocess.html

ci, msvc: Add "Run fuzz binaries" step

18fd522

hebasto force-pushed the 240331-msvc-fuzz branch from 37cda55 to 18fd522 Compare April 18, 2024 09:28

DrahtBot requested a review from sipsorcery April 26, 2024 20:12

hebasto added the Needs CMake port label Apr 26, 2024

sipa reviewed Apr 26, 2024

View reviewed changes

fanquake merged commit 3aaf732 into bitcoin:master Apr 28, 2024
16 checks passed

hebasto deleted the 240331-msvc-fuzz branch April 28, 2024 05:23

hebasto mentioned this pull request Apr 28, 2024

msvc: Compile test\fuzz\bitdeque.cpp #29983

Merged

hebasto mentioned this pull request Apr 29, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#179

Closed

hebasto added the Windows label May 1, 2024

hebasto mentioned this pull request May 1, 2024

cmake: Port PR29774 from the master branch hebasto/bitcoin#182

Merged

hebasto removed the Needs CMake port label May 1, 2024

hebasto mentioned this pull request May 3, 2024

msvc: Compile test\fuzz\miniscript.cpp #30031

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: Enable fuzz binary in MSVC #29774

build: Enable fuzz binary in MSVC #29774

hebasto commented Mar 31, 2024 •

edited

DrahtBot commented Mar 31, 2024 •

edited

hebasto commented Mar 31, 2024

sipa commented Mar 31, 2024

hebasto commented Mar 31, 2024

Richlilyjoy100 commented Mar 31, 2024

sipa commented Apr 1, 2024

sipsorcery commented Apr 1, 2024

hebasto commented Apr 1, 2024

dergoegge commented Apr 5, 2024

sipsorcery commented Apr 5, 2024

hebasto commented Apr 6, 2024

hebasto commented Apr 6, 2024 •

edited

sipsorcery commented Apr 6, 2024

sipsorcery commented Apr 6, 2024

hebasto commented Apr 6, 2024

hebasto commented Apr 6, 2024 •

edited

maflcko commented Apr 18, 2024

hebasto commented Apr 18, 2024

maflcko commented Apr 18, 2024

hebasto commented Apr 18, 2024

sipsorcery commented Apr 26, 2024

hebasto commented Apr 26, 2024

sipsorcery commented Apr 26, 2024 •

edited

sipa left a comment

hebasto commented Apr 28, 2024

hebasto commented May 1, 2024

hebasto commented May 3, 2024

build: Enable fuzz binary in MSVC #29774

build: Enable fuzz binary in MSVC #29774

Conversation

hebasto commented Mar 31, 2024 • edited

DrahtBot commented Mar 31, 2024 • edited

Code Coverage

Reviews

Conflicts

hebasto commented Mar 31, 2024

sipa commented Mar 31, 2024

hebasto commented Mar 31, 2024

Richlilyjoy100 commented Mar 31, 2024

sipa commented Apr 1, 2024

sipsorcery commented Apr 1, 2024

hebasto commented Apr 1, 2024

dergoegge commented Apr 5, 2024

sipsorcery commented Apr 5, 2024

hebasto commented Apr 6, 2024

hebasto commented Apr 6, 2024 • edited

sipsorcery commented Apr 6, 2024

sipsorcery commented Apr 6, 2024

hebasto commented Apr 6, 2024

hebasto commented Apr 6, 2024 • edited

maflcko commented Apr 18, 2024

hebasto commented Apr 18, 2024

maflcko commented Apr 18, 2024

hebasto commented Apr 18, 2024

sipsorcery commented Apr 26, 2024

hebasto commented Apr 26, 2024

sipsorcery commented Apr 26, 2024 • edited

sipa left a comment

Choose a reason for hiding this comment

hebasto commented Apr 28, 2024

hebasto commented May 1, 2024

hebasto commented May 3, 2024

hebasto commented Mar 31, 2024 •

edited

DrahtBot commented Mar 31, 2024 •

edited

hebasto commented Apr 6, 2024 •

edited

hebasto commented Apr 6, 2024 •

edited

sipsorcery commented Apr 26, 2024 •

edited