test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds #29357

hebasto · 2024-01-31T16:09:21Z

The MinGW-w64 toolchain links executables to the old msvcrt C Runtime Library that does not support the x modifier for the _wfopen() function.

Fixes #29014.

DrahtBot · 2024-01-31T16:09:24Z

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, fanquake

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

maflcko · 2024-01-31T16:18:00Z

However, the latter is available on Windows 10 only, which breaks our release compatibility with the older Windows versions.

No? According to the link you provided:

The Universal CRT is a component of the Windows operating system. It is included as a part of Windows 10, starting with the January Technical Preview, and it is available for older versions of the operating system via Windows Update.

maflcko · 2024-01-31T16:19:45Z

Nevertheless, the second commit introduces an assertion in the AutoFile constructor.

Can you explain why this change makes sense? AutoFile already checks for nullptr.

hebasto · 2024-01-31T16:22:50Z

Nevertheless, the second commit introduces an assertion in the AutoFile constructor.

Can you explain why this change makes sense? AutoFile already checks for nullptr.

The added assertion makes the issue with the unsupported x modifier explicit:

The streams_tests/xor_file test will fail without the first commit.

maflcko · 2024-01-31T16:24:32Z

It is already explicit and will fail on master without the first commit:

unknown location(0): fatal error: in "streams_tests/xor_file": std::ios_base::failure[abi:cxx11]: AutoFile::write: file handle is nullptr: iostream error

DrahtBot · 2024-01-31T16:30:24Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/21071017851}

fanquake · 2024-01-31T16:31:52Z

Concept NACK. I don't think we should remove test coverage from all other platforms, just to accomodate Windows. In the worse case this should just have a Windows paths with the modified modifiers, with documentation explaining why?

hebasto · 2024-01-31T16:38:29Z

Concept NACK. I don't think we should remove test coverage from all other platforms, just to accomodate Windows.

It is not about Windows as a platform. It is about Windows users who download the Bitcoin Core release binaries. We should not treat them in a discriminatory way.

UPD. There are no cases where the "wbx" mode is used in the non-testing code, right?

hebasto · 2024-01-31T16:40:10Z

It is already explicit and will fail on master without the first commit:
unknown location(0): fatal error: in "streams_tests/xor_file": std::ios_base::failure[abi:cxx11]: AutoFile::write: file handle is nullptr: iostream error

On the master branch the test passes on my system (Ubuntu 22.04):

$ ./src/test/test_bitcoin.exe -t streams_tests/xor_file
Running 1 test case...
00f0:err:msvcrt:msvcrt_get_flags incorrect mode flag: x

*** No errors detected

maflcko · 2024-01-31T16:43:37Z

It is already explicit and will fail on master without the first commit:
unknown location(0): fatal error: in "streams_tests/xor_file": std::ios_base::failure[abi:cxx11]: AutoFile::write: file handle is nullptr: iostream error
On the master branch the test passes on my system (Ubuntu 22.04):
$ ./src/test/test_bitcoin.exe -t streams_tests/xor_file
Running 1 test case...
00f0:err:msvcrt:msvcrt_get_flags incorrect mode flag: x

*** No errors detected

Can you add exact steps to reproduce? How is it possible that the file is a nullptr and the std::fwrite succeeds?

fanquake · 2024-01-31T16:46:00Z

We should not treat them in a discriminatory way.

I don't see how fixing the tests in the way I've suggested would be discriminatory. It would make them work on Windows while retaining the current test for all other platforms.

hebasto · 2024-01-31T16:52:04Z

Can you add exact steps to reproduce?

$ cat /etc/os-release | head -1
PRETTY_NAME="Ubuntu 22.04.3 LTS"
$ x86_64-w64-mingw32-g++-posix --version
x86_64-w64-mingw32-g++-posix (GCC) 10-posix 20220113
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ make -C depends -j $(nproc) HOST=x86_64-w64-mingw32 NO_QT=1
$ ./autogen.sh && ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site
$ make -j $(nproc) -C src test/test_bitcoin.exe
$ ./src/test/test_bitcoin.exe -t streams_tests/xor_file
Running 1 test case...
00f0:err:msvcrt:msvcrt_get_flags incorrect mode flag: x

*** No errors detected

hebasto · 2024-01-31T17:16:23Z

We should not treat them in a discriminatory way.

I don't see how fixing the tests in the way I've suggested would be discriminatory. It would make them work on Windows while retaining the current test for all other platforms.

Thank you! Your suggestion has been implemented.

maflcko · 2024-01-31T17:54:27Z

Can you add exact steps to reproduce?

$ cat /etc/os-release | head -1
PRETTY_NAME="Ubuntu 22.04.3 LTS"
$ x86_64-w64-mingw32-g++-posix --version
x86_64-w64-mingw32-g++-posix (GCC) 10-posix 20220113
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ make -C depends -j $(nproc) HOST=x86_64-w64-mingw32 NO_QT=1
$ ./autogen.sh && ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site
$ make -j $(nproc) -C src test/test_bitcoin.exe
$ ./src/test/test_bitcoin.exe -t streams_tests/xor_file
Running 1 test case...
00f0:err:msvcrt:msvcrt_get_flags incorrect mode flag: x

*** No errors detected

It prints "No errors detected", which means that the test passed.

I also tried this locally, and the file is not a nullptr.

hebasto · 2024-01-31T18:07:30Z

I also tried this locally, and the file is not a nullptr.

I agree. That is how it works on Linux in the Wine environment.

On Windows, being linked to msvcrt.dll, it fails.

maflcko · 2024-01-31T18:19:18Z

I also tried this locally, and the file is not a nullptr.

I agree. That is how it works on Linux in the Wine environment.

On Windows, being linked to msvcrt.dll, it fails.

Yes, either the file is a nullptr, in which case the test already fails, or it is not, in which case the test passes. In any case, the added assert in the dropped commit isn't needed here or elsewhere, and doesn't change the outcome here.

hebasto · 2024-01-31T18:41:16Z

I also tried this locally, and the file is not a nullptr.

I agree. That is how it works on Linux in the Wine environment.
On Windows, being linked to msvcrt.dll, it fails.

Yes, either the file is a nullptr, in which case the test already fails, or it is not, in which case the test passes. In any case, the added assert in the dropped commit isn't needed here or elsewhere, and doesn't change the outcome here.

You are right. That commit was my mistake.

FWIW, the _wfopen_s function reports an error for the x modifier even in Wine.

fanquake · 2024-02-02T11:46:07Z

src/test/streams_tests.cpp

+        // function. It is possible to link to the new UCRT library instead.
+        // However, the most easiest way to do it is to use a new GCC with
+        // the -mcrtdll=ucrt option, which has been available since the commit
+        // https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=453cb585f0f8673a5d69d1b420ffd4b3f53aca00


I think this comment could just be something like Our usage of mingw-w64 and the msvcrt runtime does not support the x modifier for the _wfopen(). The other info seems a bit overkiil, I'm also not sure if it's correct. From a quick look, if you choose the runtime you want, when you compile mingw-w64, there's no need for any GCC flag.

The other info seems a bit overkiil, I'm also not sure if it's correct. From a quick look, if you choose the runtime you want, when you compile mingw-w64, there's no need for any GCC flag.

If the new runtime can be linked in guix builds, I'd say it would be better to use it, instead of patching the code. Or is there some downside I am missing?

If the new runtime is available in guix, I'd say it would be better to just use it, instead of patching the code. Or is there some downside I am missing?

I agree that we should move to the new runtime. I assume the downside is possibly dropping support for Windows 7, but I don't see why that would be a problem.

According to the docs, if it is missing, it can be installed via "Windows Update": https://devblogs.microsoft.com/cppblog/introducing-the-universal-crt/ . IIUC it is shipped by default on Windows 10.

if you choose the runtime you want, when you compile mingw-w64

Yes, there is --with-default-msvcrt=ucrt configuration option.

Yea. So I don't see why anything GCC related is required, or needs to be mentioned here.

The following patch

--- a/contrib/guix/manifest.scm +++ b/contrib/guix/manifest.scm @@ -108,6 +108,16 @@ desirable for building Bitcoin Core release binaries." base-libc base-gcc)) + +(define-public mingw-w64-x86_64-winpthreads-ucrt + (package + (inherit mingw-w64-x86_64-winpthreads) + (arguments + (substitute-keyword-arguments (package-arguments mingw-w64-x86_64-winpthreads) + ((#:configure-flags flags) + `(append ,flags + '("--with-default-msvcrt=ucrt"))))))) + (define (gcc-mingw-patches gcc) (package-with-extra-patches gcc (search-our-patches "gcc-remap-guix-store.patch" @@ -116,7 +126,7 @@ desirable for building Bitcoin Core release binaries." (define (make-mingw-pthreads-cross-toolchain target) "Create a cross-compilation toolchain package for TARGET" (let* ((xbinutils (cross-binutils target)) - (pthreads-xlibc mingw-w64-x86_64-winpthreads) + (pthreads-xlibc mingw-w64-x86_64-winpthreads-ucrt) (pthreads-xgcc (cross-gcc target #:xgcc (gcc-mingw-patches mingw-w64-base-gcc) #:xbinutils xbinutils

results in an error:

x86_64-w64-mingw32-ld: src/.libs/libwinpthread_la-thread.o: in function `pthread_create_wrapper': /tmp/guix-build-mingw-w64-x86_64-winpthreads-11.0.1.drv-0/mingw-w64-v11.0.1/mingw-w64-libraries/winpthreads/src/thread.c:1518: undefined reference to `__intrinsic_setjmpex' collect2: error: ld returned 1 exit status

Any ideas how to resolve it?

fanquake · 2024-02-02T11:47:59Z

FWIW, the _wfopen_s function reports an error for the x modifier even in Wine.

So it reports an error but the unit tests don't fail? Is this another (different bug)?

hebasto · 2024-02-02T11:50:47Z

FWIW, the _wfopen_s function reports an error for the x modifier even in Wine.

So it reports an error but the unit tests don't fail? Is this another (different bug)?

We use the _wfopen. Microsoft docs suggest:

More-secure versions of these functions that perform more parameter validation and return error codes are available; see fopen_s, _wfopen_s.

So, the _wfopen_s is an alternative, which is not used now.

UPD. I've made a notice about a potential code change.

DrahtBot · 2024-02-03T00:48:31Z

Guix builds (on x86_64)

File	commit `5b8c597` (master)	commit `f381e30` (master and this pull)
SHA256SUMS.part	`57c6ab3b21eb3a09...`	`67b09cfb1d4e19ec...`
*-aarch64-linux-gnu-debug.tar.gz	`be53953269a8ac42...`	`72f9e3b208e18cf0...`
*-aarch64-linux-gnu.tar.gz	`41bb7a5fb14be431...`	`2aca20e9a358e12c...`
*-arm-linux-gnueabihf-debug.tar.gz	`10e78e10babe7504...`	`22c967e800f480f3...`
*-arm-linux-gnueabihf.tar.gz	`fad8f69e29ddc87b...`	`f51c5b9b57d2c69b...`
*-arm64-apple-darwin-unsigned.tar.gz	`57ae6228771a65a0...`	`6d4af8dba59aab48...`
*-arm64-apple-darwin-unsigned.zip	`cf15574b2779ef73...`	`dcdb3606509f9cf2...`
*-arm64-apple-darwin.tar.gz	`ca697a12e89eaa55...`	`d9a673bf030ec52d...`
*-powerpc64-linux-gnu-debug.tar.gz	`943c67610f1d4702...`	`d06f837549fc5a44...`
*-powerpc64-linux-gnu.tar.gz	`cf28170ae00aee83...`	`3e6ab9b6cf1fab9d...`
*-powerpc64le-linux-gnu-debug.tar.gz	`f71559e0658bec70...`	`7653256f74aa764e...`
*-powerpc64le-linux-gnu.tar.gz	`0ea2ad98f028654f...`	`35493f06f91573bd...`
*-riscv64-linux-gnu-debug.tar.gz	`1e87775ff9231cd0...`	`3263b33eb86f4ce8...`
*-riscv64-linux-gnu.tar.gz	`a8612d1adb2155da...`	`f8bddc3e1c4301c5...`
*-x86_64-apple-darwin-unsigned.tar.gz	`bb8c8cbe3fea1fc8...`	`ee6d2dcfc760ddfb...`
*-x86_64-apple-darwin-unsigned.zip	`ac3b4965cb983977...`	`dd356f118bbfb286...`
*-x86_64-apple-darwin.tar.gz	`203c8fb15312f7e4...`	`6fddd16b4f04954b...`
*-x86_64-linux-gnu-debug.tar.gz	`a16d15d38419bce6...`	`f08936a087c189c2...`
*-x86_64-linux-gnu.tar.gz	`13043b9495c8e848...`	`1353eeb12f4ced9c...`
*.tar.gz	`e84c2a3741aa5533...`	`3b18fcb162123afb...`
guix_build.log	`b2335404ab6a37db...`	`b54dd0996898d53b...`
guix_build.log.diff		`53757ba83f2f33e3...`

maflcko · 2024-02-23T11:29:43Z

I haven't reviewed this in detail, but it seems good enough for a temporary workaround.

lgtm ACK d4690e4

fanquake · 2024-02-26T14:44:29Z

I think the documentation should still be adjusted (#29357 (comment)). No point merging what I still think are incorrect docs, especially if the change is going to be backported. In any case, our unit test code also seems like the wrong place to put overly verbose documentaiton about GCC configuration options and the various windows runtimes.

The MinGW-w64 toolchain links executables to the old msvcrt C Runtime Library that does not support the `x` modifier for the _wfopen() function.

hebasto · 2024-02-26T14:48:48Z

I think the documentation should still be adjusted (#29357 (comment)).

Sorry, it skipped my attention. Fixed now.

maflcko · 2024-02-26T14:55:26Z

ACK d2fe905

fanquake

ACK d2fe905 - the plan here should still be to migrate to the newer windows runtime.

pablomartin4btc

post-merge ACK d2fe905

I've tested this fix earlier on the related issue but missed the PR.

The MinGW-w64 toolchain links executables to the old msvcrt C Runtime Library that does not support the `x` modifier for the _wfopen() function. Github-Pull: bitcoin#29357 Rebased-From: d2fe905

fanquake · 2024-03-04T11:39:43Z

Backported for 26.x in #29509.

c68d4d0 [doc] update manual pages for v26.1rc2 (glozow) bd715bf [build] bump version to v26.1rc2 (glozow) b6d006d update release notes 26.1 (glozow) fce992b fuzz: restrict fopencookie usage to Linux & FreeBSD (fanquake) 40c56a4 test: make sure keypool sizes do not change on `getrawchangeaddress`/`getnewaddress` failures (UdjinM6) 7c82b27 wallet: Avoid updating `ReserveDestination::nIndex` when `GetReservedDestination` fails (UdjinM6) b5419ce p2p: Don't consider blocks mutated if they don't connect to known prev block (Greg Sanders) 0535c25 [test] IsBlockMutated unit tests (dergoegge) 8141498 [validation] Cache merkle root and witness commitment checks (dergoegge) 0c5c596 [test] Add regression test for #27608 (dergoegge) 2473635 [net processing] Don't process mutated blocks (dergoegge) 50c0b61 [validation] Merkle root malleation should be caught by IsBlockMutated (dergoegge) aff368f [validation] Introduce IsBlockMutated (dergoegge) 076c67c [refactor] Cleanup merkle root checks (dergoegge) 97a1d0a [validation] Isolate merkle root checks (dergoegge) 4ac0eb5 test: Drop `x` modifier in `fsbridge::fopen` call for mingw builds (Hennadii Stepanov) Pull request description: Includes: - #29357 - #29412 - #29524 - #29510 - #29529 Also does: - update to release notes - bump to rc2 - manpages - (no changes to bitcoin.conf) ACKs for top commit: achow101: ACK c68d4d0 Tree-SHA512: 2f8c3dd705e3f9f33403b3cc17e8006510ff827d7dbd609b09732a1669964e9b001cfecdc63d8d8daeb8f39c652e1e4ad0aac873d44d259c21803de85688ed2b

DrahtBot added the Tests label Jan 31, 2024

hebasto force-pushed the 240131-fopen-x branch from fa02938 to bb79fd6 Compare January 31, 2024 16:30

DrahtBot added the CI failed label Jan 31, 2024

hebasto force-pushed the 240131-fopen-x branch from bb79fd6 to 9c1042a Compare January 31, 2024 17:15

hebasto force-pushed the 240131-fopen-x branch from 9c1042a to d4690e4 Compare January 31, 2024 17:17

hebasto changed the title ~~test: Drop x modifier in fsbridge::fopen call~~ test: Drop x modifier in fsbridge::fopen call for MinGW builds Jan 31, 2024

hebasto added the Windows label Jan 31, 2024

DrahtBot removed the CI failed label Jan 31, 2024

maflcko added the Needs backport (26.x) label Feb 1, 2024

maflcko added this to the 26.1 milestone Feb 1, 2024

maflcko added the DrahtBot Guix build requested label Feb 2, 2024

fanquake reviewed Feb 2, 2024

View reviewed changes

DrahtBot removed the DrahtBot Guix build requested label Feb 3, 2024

achow101 modified the milestones: 26.1, 27.0 Feb 15, 2024

test: Drop x modifier in fsbridge::fopen call for mingw builds

d2fe905

The MinGW-w64 toolchain links executables to the old msvcrt C Runtime Library that does not support the `x` modifier for the _wfopen() function.

hebasto force-pushed the 240131-fopen-x branch from d4690e4 to d2fe905 Compare February 26, 2024 14:47

fanquake approved these changes Feb 26, 2024

View reviewed changes

fanquake merged commit 4d7d7fd into bitcoin:master Feb 26, 2024

hebasto deleted the 240131-fopen-x branch February 26, 2024 16:24

pablomartin4btc reviewed Feb 26, 2024

View reviewed changes

glozow mentioned this pull request Feb 28, 2024

[26.x] backports and final changes for v26.1rc2 #29509

Merged

fanquake removed the Needs backport (26.x) label Mar 4, 2024

fanquake mentioned this pull request May 31, 2024

build: use UCRT runtime for Windows (release) binaries #30210

Open

6 tasks

bitcoin deleted a comment Nov 19, 2024

test: Drop x modifier in fsbridge::fopen call for MinGW builds #29357

test: Drop x modifier in fsbridge::fopen call for MinGW builds #29357

Uh oh!

Conversation

hebasto commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

maflcko commented Jan 31, 2024

Uh oh!

maflcko commented Jan 31, 2024

Uh oh!

hebasto commented Jan 31, 2024

Uh oh!

maflcko commented Jan 31, 2024

Uh oh!

DrahtBot commented Jan 31, 2024

Uh oh!

fanquake commented Jan 31, 2024

Uh oh!

hebasto commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jan 31, 2024

Uh oh!

fanquake commented Jan 31, 2024

Uh oh!

hebasto commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 31, 2024

Uh oh!

maflcko commented Jan 31, 2024

Uh oh!

hebasto commented Jan 31, 2024

Uh oh!

maflcko commented Jan 31, 2024

Uh oh!

hebasto commented Jan 31, 2024

Uh oh!

fanquake Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

fanquake Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Feb 2, 2024

Uh oh!

hebasto commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 3, 2024

Guix builds (on x86_64)

Uh oh!

maflcko commented Feb 23, 2024

Uh oh!

test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds #29357

test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds #29357

hebasto commented Jan 31, 2024 •

edited

Loading

DrahtBot commented Jan 31, 2024 •

edited

Loading

hebasto commented Jan 31, 2024 •

edited

Loading

hebasto commented Jan 31, 2024 •

edited

Loading

hebasto commented Jan 31, 2024 •

edited

Loading

maflcko Feb 2, 2024 •

edited

Loading

hebasto Feb 2, 2024 •

edited

Loading

hebasto commented Feb 2, 2024 •

edited

Loading