Skip to content
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

cmake: fix unity with Windows Unicode + TrackMemory #11928

Closed
wants to merge 1 commit into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Sep 24, 2023

Found the root cause of the startup crash in unity builds with Unicode
and TrackMemory enabled at the same time.

We must make sure that the memdebug.h header doesn't apply to
lib/curl_multibyte.c (as even noted in a comment there.) In unity
builds all headers apply to all sources, including curl_multibyte.c.
This probably resulted in an infinite loop on startup.

Exclude this source from unity compilation with TrackMemory enabled,
in both libcurl and curl tool. Enable unity mode for a debug Unicode
CI job to keep it tested. Also delete the earlier workaround that
fully disabled unity for affected builds.

Follow-up to d82b080 #12005
Follow-up to 3f8fc25 #11095

Closes #11928


Crashing on startup example:

could not get curl version! at C:/projects/curl/tests/runtests.pl line 2439.

https://ci.appveyor.com/project/curlorg/curl/builds/48113110/job/3ou611asxeop6uvi#L2522

@github-actions github-actions bot added CI Continuous Integration cmdline tool labels Sep 24, 2023
@vszakats vszakats marked this pull request as draft September 24, 2023 23:23
@vszakats vszakats removed the tests label Sep 25, 2023
@github-actions github-actions bot added the tests label Sep 25, 2023
@vszakats vszakats changed the title cmake: fix stderr init failure with unity shared debug curl too appveyor: debug why curl fails to run for tests Sep 25, 2023
@vszakats vszakats changed the title appveyor: debug why curl fails to run for tests ci: debug why curl fails to run for tests Sep 25, 2023
@vszakats vszakats changed the title ci: debug why curl fails to run for tests ci: debug why shared curl fails to run for tests Sep 25, 2023
@vszakats vszakats force-pushed the fix-unity-no-stderr branch 4 times, most recently from 818f5c5 to 3788015 Compare September 27, 2023 09:48
@vszakats
Copy link
Member Author

vszakats commented Sep 27, 2023

So, for one thing, libtests and curl tool wasn't finding libcurl DLL, that was the next issue. After fixing that, the non-unity builds started running as expected, but their unity counterparts kept failing. They run fine locally with wine64, but crash on startup on real Windows with 0xc0000374 (heap corruption) in "StackHash_a77e". [ Unity binaries are also much smaller, but I attribute that to the more concise debug info in the unity builds. Their size become similar after stripping. ]

So the next issue is to find what causes these shared/debug unity builds to fail on real Windows.

UPDATE: The issue doesn't happen with non-unicode builds:
good: https://ci.appveyor.com/project/curlorg/curl/builds/48142669/job/l64j23l9sk4jsq2p/artifacts
bad: https://ci.appveyor.com/project/curlorg/curl/builds/48142669/job/hup9wvbbiy1krhfo/artifacts

UPDATE 2: The crashing curl tool happens with unicode + debug + unity (tool) builds (both static and shared): https://ci.appveyor.com/project/curlorg/curl/builds/48143670

@vszakats vszakats force-pushed the fix-unity-no-stderr branch 8 times, most recently from b13738c to fe90a01 Compare October 1, 2023 20:13
@vszakats
Copy link
Member Author

vszakats commented Oct 1, 2023

curl_memory.h, curl_multibyte.h and memdebug.h look suspect, but no evidence or test result.

I'm about to give up finding the issue, and instead disable UNITY for src when ENABLE_UNICODE=ON and ENABLE_DEBUG=ON (= ENABLE_CURLDEBUG=ON = -DCURLDEBUG = TrackMemory) are enabled. It doesn't look a terrible limitation.

UPDATE: It was curl_multibyte.c.

vszakats added a commit to vszakats/curl that referenced this pull request Oct 2, 2023
"TrackMemory" is `ENABLE_DEBUG=ON` (aka `ENABLE_CURLDEBUG=ON`,
aka `-DCURLDEBUG`).

There is an issue with memory tracking and Unicode when build in "unity"
mode, which results in the curl tool crashing right on startup, even
without any command-line option. Interestingly this doesn't happen under
WINE (at least on the system I tested this on), but consistenly happens
on real Windows machines. Crash is 0xC0000374 heap corruption. Both
shared and static curl executables are affected.

This limitation probably won't hit too many people, but it remains
a TODO to find and fix the root cause and drop this workaround.

Example builds and runs:
https://ci.appveyor.com/project/curlorg/curl/builds/48169111/job/17cptxhtpubd7iwj#L313 (static)
https://ci.appveyor.com/project/curlorg/curl/builds/48169111/job/76e1ge758tbyqu9c#L317 (shared)

Follow-up to 3f8fc25 curl#11095

Ref curl#11928
Closes #xxxxx
vszakats added a commit that referenced this pull request Oct 2, 2023
"TrackMemory" is `ENABLE_DEBUG=ON` (aka `ENABLE_CURLDEBUG=ON`,
aka `-DCURLDEBUG`).

There is an issue with memory tracking and Unicode when built in "unity"
mode, which results in the curl tool crashing right on startup, even
without any command-line option. Interestingly this doesn't happen under
WINE (at least on the system I tested this on), but consistenly happens
on real Windows machines. Crash is 0xC0000374 heap corruption. Both
shared and static curl executables are affected.

This limitation probably won't hit too many people, but it remains
a TODO to find and fix the root cause and drop this workaround.

Example builds and runs:
https://ci.appveyor.com/project/curlorg/curl/builds/48169111/job/17cptxhtpubd7iwj#L313 (static)
https://ci.appveyor.com/project/curlorg/curl/builds/48169111/job/76e1ge758tbyqu9c#L317 (shared)

Follow-up to 3f8fc25 #11095

Ref: #11928
Closes #12005
vszakats added a commit to vszakats/curl that referenced this pull request Oct 3, 2023
Found the root cause of the startup crash with unity builds with Unicode
and TrackMemory enabled at the same time.

We must make sure that the `memdebug.h` header doesn't apply to
`lib/curl_multibyte.c` (as even noted in a comment there.). In unity
builds all headers apply to all sources, including `curl_multibyte.c`.
This probably resulted in an infinite loop on startup.

This patch excludes this source from unity compilation with TrackMemory
enabled, in both libcurl and curl tool. Also delete the earlier
workaround that fully disabled unity for affected builds.

Also enable unity mode for a debug unicode CI job.

Follow-up to d82b080 curl#12005

Closes curl#11928
@vszakats vszakats changed the title ci: debug why shared curl fails to run for tests cmake: fix unity mode with Windows Unicode + TrackMemory Oct 3, 2023
@vszakats vszakats changed the title cmake: fix unity mode with Windows Unicode + TrackMemory cmake: fix unity with Windows Unicode + TrackMemory Oct 3, 2023
Found the root cause of the startup crash with unity builds with Unicode
and TrackMemory enabled at the same time.

We must make sure that the `memdebug.h` header doesn't apply to
`lib/curl_multibyte.c` (as even noted in a comment there.). In unity
builds all headers apply to all sources, including `curl_multibyte.c`.
This probably resulted in an infinite loop on startup.

This patch excludes this source from unity compilation with TrackMemory
enabled, in both libcurl and curl tool. Also delete the earlier
workaround that fully disabled unity for affected builds.

Also enable unity mode for a debug unicode CI job.

Follow-up to d82b080 curl#12005
Follow-up to 3f8fc25 curl#11095

Closes curl#11928
@vszakats vszakats added Windows Windows-specific unicode Unicode, code page, character encoding and removed cmdline tool labels Oct 3, 2023
@vszakats vszakats marked this pull request as ready for review October 3, 2023 02:53
@jay
Copy link
Member

jay commented Oct 3, 2023

CMake, mingw-w64, gcc 8, Debug x64, Schannel, Static, Unicode, Unity (the build you just enabled unity on) failed in ci due to an ftp test. ftp testing in windows has some flakiness (#12002 for example) so I suspect it's unrelated but I set the job to rerun

@vszakats
Copy link
Member Author

vszakats commented Oct 3, 2023

The way to track this down was not the most obvious. While doing the first tests I added a way to run curl.exe after each build. This turned out to be non-trivial with the MS-DOS batch, triggering a rewrite in PowerShell that is better, and going into an all new rabbit hole (#11999). But, it resulted in CI jobs able to detect this issue, a more clearly and much quicker than ./runtests.pl failing with could not get curl version!. Locally, it needed a real Windows VM for running curl.exe. Strangely WINE was able to run these filaing curl.exes without issues.

Then it took a long series of builds to narrow this down the mininum conditions.

When this cleared up, I had some guesses where to look, but eyeballing the source didn't help. I was also looking at headers, assuming that these could affect both libcurl and curl, so missed the comment in the affected source file.

At this point I created one build with and one without the issue and planned to compare disassembled binaries. For this I had to learn how to tell CMake to use -O0 without enabling debug. This needed editing its own config file (unintuitively: .../share/cmake/Modules/Compiler/GNU.cmake, for llvm/clang) and not supported with regular CMake options or via CMakeLists.txt. But, this turned out to be a dead-end anyway. Even with non-debug -O0, the resulting objects have internal functions in different order, string optimization was still enabled and cross-references also made up too much difference in the objdump -D outputs.

Next I picked the compiler commands from the build logs and re-run them by replacing -o <obj> with -E for both builds, to compare the actual C code they're ending up with. It was interesting to realize that an almost minimal Windows curl build creates 600MB of compilation input (lib + src) without unity and 11MB (!) with unity. This explains the massive gain in compilation speed. And also the dramatic reduction in the debug info included in debug builds. A large portion of that is likely Windows headers (I haven't made tests with a Linux or macOS build):

-rw-r--r--  1 user  staff  465451081 Oct  3 10:46 lib-non_unity.c.E
-rw-r--r--  1 user  staff    8108339 Oct  3 10:44 lib-unity.c.E
-rw-r--r--  1 user  staff  134004891 Oct  3 10:47 src-non_unity.c.E
-rw-r--r--  1 user  staff    3659820 Oct  3 10:44 src-unity.c.E

Comparing texts this large was a no-go, so picked up the uninclude script from GCC, to strip system headers. Had to modify it to recognize mingw-w64 headers (x86_64-w64-mingw32/include). Since curl keeps no executable code in headers, I took a risk and also stripped include/curl and curl/lib (this will bite a bit later).

This still had too much noise, so restarted the -E pass after making sure that the order of the non-unity build sources matches the ones in Makefile.inc files. (A PR is coming up fixing a few alpha-sorting issues in those. #12014) Sorting was different because I left in -j4 for the build runs that generated the logs with the compiler commands.

At this point it was a matter of sifting through the two diffs for lib and src respectively. Disregaring the remaining header-noise, these were reasonably identical, but not completely so. Notably ((void *)0) might be ((void*)0), and ((NTSTATUS)0x00000000) is also ((NTSTATUS)0x00000000L) depending on unity and non-unity. I surmise because Windows headers' sensitivity to include order.

src diff did not reveal anything suspicious. But lib did:

Screen Shot 2023-10-03 at 11 11 50
Left: non-unity, Right: unity

The risk I took to strip curl/lib stuff from the .E output deleted the curl_multbyte.c reference from the src diffs, otherwise I could have caught that as well.

The fix itself was trivial, after realizing that we also use curl_multibyte.c for the curl tool.

@vszakats vszakats closed this in f42a279 Oct 3, 2023
@vszakats vszakats deleted the fix-unity-no-stderr branch October 3, 2023 09:45
@dfandrich
Copy link
Contributor

dfandrich commented Oct 3, 2023 via email

vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 25, 2023
Switch from `curl-gnumake.sh` to `curl-cmake.sh` with upcoming curl
release v8.5.0.

cmake builds are now _faster_ for Windows builds than raw gnumake
(aka `Makefile.mk`). They also use 'unity' mode, which makes builds
run fast with the side-effect of also creating potentially more
efficient binaries by allowing better compiler optimizations.

This also makes curl-for-win use the same build system for all target
platforms (`Makefile.mk` is not available for *nix platforms).

Initially on 2022-07-04 cmake was 25% slower than gnumake. By
2022-09-26 this reduced to 20%, by 2023-07-29 to 18% and after the
latest round of updates gnumake came out 7% slower than cmake.
This is for Windows, for a triple x64, arm64 and x86 build. In
absolute times this is 27m22s for cmake and 29m11s for gnumake.

(FWIW autotools builds are 52% slower than cmake unity builds now.)

Making cmake builds fast was a multi-year effort with these major steps:

1. add support for cmake builds in curl-for-win.
   420e73c
2. bring it in-line with gnumake and autotools builds and tidy-up
   as much as possible. Scattered to a many commits.
3. delete a whole bunch of unused feature detections.
   curl/curl@4d73854
   curl/curl#9044
   (and a lot more smaller commits)
4. speed up picky warning option initialization by avoiding brute-force
   all options. (first in libssh2, then in curl, then applied also
   ngtcp2, nghttp3, nghttp2)
   curl/curl@9c543de
   curl/curl#10973
5. implement single build run for shared and static libcurl + tool
   (first in libssh2)
   curl/curl@1199308
   curl/curl#11505
   53dcd49
6. implement single build pass for shared and static libcurl
   (for Windows initially)
   curl/curl@2ebc74c
   curl/curl#11546
7. extend above to non-Windows (e.g. macOS)
   curl/curl@fc9bfb1
   curl/curl#11627
   bafa77d (mac)
   1b27304 (linux)
8. implement unity builds: single compiler invocation for libcurl + tool
   curl/curl@3f8fc25
   curl/curl#11095
   curl/curl@f42a279
   curl/curl#11928
   67d7fd3
9. speed up 4x the cmake initialization phase (for Windows)
   curl/curl@2100d9f
   curl/curl#12044
10. speed up cmake initialization even more by pre-filling
   detection results for our well-known mingw-w64 environments.
   74dd967
   5a43c61

+1: speed up autotools initialization by mitigating a brute-force
   feature detection on Windows. This reduced total build times
   by 5 minutes at the time, for the 3 Windows targets in total.
   curl/curl@6adcff6
   curl/curl#9591

Also update build times for cmake-unity and gnumake, based on runs:
cmake-unity: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/48357875
gnumake: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/48358005
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration cmake tests unicode Unicode, code page, character encoding Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

4 participants