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

hebasto commented at 4:09 pm on January 31, 2024: member

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 commented at 4:09 pm on January 31, 2024: contributor

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.

DrahtBot added the label Tests on Jan 31, 2024

maflcko commented at 4:18 pm on January 31, 2024: member

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 commented at 4:19 pm on January 31, 2024: member

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 commented at 4:22 pm on January 31, 2024: member

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 commented at 4:24 pm on January 31, 2024: member

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

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

hebasto force-pushed on Jan 31, 2024

DrahtBot added the label CI failed on Jan 31, 2024

DrahtBot commented at 4:30 pm on January 31, 2024: contributor

🚧 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 commented at 4:31 pm on January 31, 2024: member

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 commented at 4:38 pm on January 31, 2024: member

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 commented at 4:40 pm on January 31, 2024: member

It is already explicit and will fail on master without the first commit:
0unknown 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):

0$ ./src/test/test_bitcoin.exe -t streams_tests/xor_file
1Running 1 test case...
200f0:err:msvcrt:msvcrt_get_flags incorrect mode flag: x
3
4*** No errors detected

maflcko commented at 4:43 pm on January 31, 2024: member

It is already explicit and will fail on master without the first commit:
0unknown 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):

0$ ./src/test/test_bitcoin.exe -t streams_tests/xor_file
1Running 1 test case...
200f0:err:msvcrt:msvcrt_get_flags incorrect mode flag: x
3
4*** 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 commented at 4:46 pm on January 31, 2024: member

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 commented at 4:52 pm on January 31, 2024: member

Can you add exact steps to reproduce?

 0$ cat /etc/os-release | head -1
 1PRETTY_NAME="Ubuntu 22.04.3 LTS"
 2$ x86_64-w64-mingw32-g++-posix --version
 3x86_64-w64-mingw32-g++-posix (GCC) 10-posix 20220113
 4Copyright (C) 2020 Free Software Foundation, Inc.
 5This is free software; see the source for copying conditions.  There is NO
 6warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 7
 8$ make -C depends -j $(nproc) HOST=x86_64-w64-mingw32 NO_QT=1
 9$ ./autogen.sh && ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site
10$ make -j $(nproc) -C src test/test_bitcoin.exe
11$ ./src/test/test_bitcoin.exe -t streams_tests/xor_file
12Running 1 test case...
1300f0:err:msvcrt:msvcrt_get_flags incorrect mode flag: x
14
15*** No errors detected

hebasto force-pushed on Jan 31, 2024

hebasto commented at 5:16 pm on January 31, 2024: member

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.

hebasto force-pushed on Jan 31, 2024

hebasto renamed this:
~~test: Drop `x` modifier in `fsbridge::fopen` call~~
test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds
on Jan 31, 2024

maflcko commented at 5:54 pm on January 31, 2024: member

Can you add exact steps to reproduce?

 0$ cat /etc/os-release | head -1
 1PRETTY_NAME="Ubuntu 22.04.3 LTS"
 2$ x86_64-w64-mingw32-g++-posix --version
 3x86_64-w64-mingw32-g++-posix (GCC) 10-posix 20220113
 4Copyright (C) 2020 Free Software Foundation, Inc.
 5This is free software; see the source for copying conditions.  There is NO
 6warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 7
 8$ make -C depends -j $(nproc) HOST=x86_64-w64-mingw32 NO_QT=1
 9$ ./autogen.sh && ./configure CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site
10$ make -j $(nproc) -C src test/test_bitcoin.exe
11$ ./src/test/test_bitcoin.exe -t streams_tests/xor_file
12Running 1 test case...
1300f0:err:msvcrt:msvcrt_get_flags incorrect mode flag: x
14
15*** 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 commented at 6:07 pm on January 31, 2024: member

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 commented at 6:19 pm on January 31, 2024: member

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 commented at 6:41 pm on January 31, 2024: member

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.

hebasto added the label Windows on Jan 31, 2024

DrahtBot removed the label CI failed on Jan 31, 2024

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

maflcko added this to the milestone 26.1 on Feb 1, 2024

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

in src/test/streams_tests.cpp:38 in d4690e449a outdated

34+        // The MinGW-w64 toolchain links executables to the old msvcrt C Runtime
35+        // Library that does not support the `x` modifier for the _wfopen()
36+        // function. It is possible to link to the new UCRT library instead.
37+        // However, the most easiest way to do it is to use a new GCC with
38+        // the -mcrtdll=ucrt option, which has been available since the commit
39+        // https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=453cb585f0f8673a5d69d1b420ffd4b3f53aca00

fanquake commented at 11:46 am on February 2, 2024:

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.

maflcko commented at 11:51 am on February 2, 2024:

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?

fanquake commented at 11:52 am on February 2, 2024:

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.

maflcko commented at 11:56 am on February 2, 2024:

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.

hebasto commented at 12:00 pm on February 2, 2024:

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

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

fanquake commented at 12:38 pm on February 2, 2024:

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

hebasto commented at 3:50 pm on February 2, 2024:

The following patch

 0--- a/contrib/guix/manifest.scm
 1+++ b/contrib/guix/manifest.scm
 2@@ -108,6 +108,16 @@ desirable for building Bitcoin Core release binaries."
 3                         base-libc
 4                         base-gcc))
 5 
 6+
 7+(define-public mingw-w64-x86_64-winpthreads-ucrt
 8+  (package
 9+    (inherit mingw-w64-x86_64-winpthreads)
10+    (arguments
11+      (substitute-keyword-arguments (package-arguments mingw-w64-x86_64-winpthreads)
12+        ((#:configure-flags flags)
13+          `(append ,flags
14+                   '("--with-default-msvcrt=ucrt")))))))
15+
16 (define (gcc-mingw-patches gcc)
17   (package-with-extra-patches gcc
18     (search-our-patches "gcc-remap-guix-store.patch"
19@@ -116,7 +126,7 @@ desirable for building Bitcoin Core release binaries."
20 (define (make-mingw-pthreads-cross-toolchain target)
21   "Create a cross-compilation toolchain package for TARGET"
22   (let* ((xbinutils (cross-binutils target))
23-         (pthreads-xlibc mingw-w64-x86_64-winpthreads)
24+         (pthreads-xlibc mingw-w64-x86_64-winpthreads-ucrt)
25          (pthreads-xgcc (cross-gcc target
26                                     #:xgcc (gcc-mingw-patches mingw-w64-base-gcc)
27                                     #:xbinutils xbinutils

results in an error:

0x86_64-w64-mingw32-ld: src/.libs/libwinpthread_la-thread.o: in function `pthread_create_wrapper':
1/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'
2collect2: error: ld returned 1 exit status

Any ideas how to resolve it?

fanquake commented at 11:47 am on February 2, 2024: member

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 commented at 11:50 am on February 2, 2024: member

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 commented at 0:48 am on February 3, 2024: contributor

Guix builds (on x86_64)

File	commit 5b8c5970bdfc817cac9b59f699925c4426c59b61(master)	commit f381e3073f9fc2a9e13a459efbb94c8556247928(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...`

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

achow101 removed this from the milestone 26.1 on Feb 15, 2024

achow101 added this to the milestone 27.0 on Feb 15, 2024

maflcko commented at 11:29 am on February 23, 2024: member

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

lgtm ACK d4690e449ae470b8274176396564237ee6ec0f35

fanquake commented at 2:44 pm on February 26, 2024: member

I think the documentation should still be adjusted (https://github.com/bitcoin/bitcoin/pull/29357#discussion_r1475960179). 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.

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

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

d2fe90571e

hebasto force-pushed on Feb 26, 2024

hebasto commented at 2:48 pm on February 26, 2024: member

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

Sorry, it skipped my attention. Fixed now.

maflcko commented at 2:55 pm on February 26, 2024: member

ACK d2fe90571e98e02617682561ea82acb1e2647827

fanquake approved

fanquake commented at 4:14 pm on February 26, 2024: member

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

fanquake merged this on Feb 26, 2024

fanquake closed this on Feb 26, 2024

hebasto deleted the branch on Feb 26, 2024

pablomartin4btc commented at 4:32 pm on February 26, 2024: member

post-merge ACK d2fe90571e98e02617682561ea82acb1e2647827

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

glozow referenced this in commit 4ac0eb543d on Feb 28, 2024

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

fanquake commented at 11:39 am on March 4, 2024: member

Backported for 26.x in #29509.

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

Code Coverage

Reviews

Guix builds (on x86_64)

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