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: add support for "unity" builds #11095

Closed
wants to merge 14 commits into from
Closed

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented May 9, 2023

Aka "jumbo" or "amalgamation" builds. It means to compile all sources
per target as a single C source. This is experimental.

You can enable it by passing -DCMAKE_UNITY_BUILD=ON to cmake.
It requires CMake 3.16 or newer.

It makes builds (much) faster, allows for better optimizations and tends
to promote less ambiguous code.

Also add a new AppVeyor CI job and convert an existing one to use
"unity" mode (one MSVC, one MinGW), and enable it for one macOS CI job.

Fix related issues:

  • add missing include guard to easy_lock.h.
  • rename static variables and functions (and a macro) with names reused
    across sources, or shadowed by local variables.
  • add an #undef after use.
  • add a missing #undef before use.
  • move internal definitions from ftp.h to ftp.c.
  • curl_memory.h fixes to make it work when included repeatedly.
  • stop building/linking curlx bits twice for a static-mode curl tool.
    These caused doubly defined symbols in unity builds.
  • silence missing extern declarations compiler warning for _CRT_glob.
  • fix extern declarations for tool_freq and tool_isVistaOrGreater.
  • fix colliding static symbols in debug mode: debugtime() and
    statename.
  • rename ssl_backend_data structure to unique names for each
    TLS-backend, along with the ssl_connect_data struct member
    referencing them. This required adding casts for each access.
  • add workaround for missing [P]UNICODE_STRING types in certain Windows
    builds when compiling lib/ldap.c. To support "unity" builds, we had
    to enable SCHANNEL_USE_BLACKLISTS for Schannel (a Windows
    schannel.h option) globally. This caused an indirect inclusion of
    Windows schannel.h from ldap.c via winldap.h to have it enabled
    as well. This requires [P]UNICODE_STRING types, which is apperantly
    not defined automatically (as seen with both MSVS and mingw-w64).
    This patch includes <subauth.h> to fix it.
    Ref: https://github.com/curl/curl/runs/13987772013
    Ref: https://dev.azure.com/daniel0244/curl/_build/results?buildId=15827&view=logs&jobId=2c9f582d-e278-56b6-4354-f38a4d851906&j=2c9f582d-e278-56b6-4354-f38a4d851906&t=90509b00-34fa-5a81-35d7-5ed9569d331c
  • tweak unity builds to compile lib/memdebug.c separately in memory
    trace builds to avoid PP confusion.
  • force-disable unity for test programs.
  • do not compile and link libcurl sources to libtests twice when libcurl
    is built in static mode.

Ref: libssh2/libssh2#1034
Ref: https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD.html
Ref: https://en.wikipedia.org/wiki/Unity_build

Closes #11095

@vszakats vszakats added cmake feature-window A merge of this requires an open feature window performance labels May 9, 2023
@vszakats
Copy link
Member Author

vszakats commented May 9, 2023

Difficulties with H2, H2-proxy and H1-proxy name collisions, and CF_CTX_CALL_DATA vs. CF_DATA_CURRENT in particular.

The bigger hurdle is MultiSSL, especially ssl_backend_data, which is a name used for both type and variable and some other things I've yet to understand.

("unity" builds require unique symbol names across sources, including statics, types, and macros.)

@jay
Copy link
Member

jay commented May 10, 2023

Is this worth the added maintenance and couldn't it introduce bugs? If we redefine a macro in one unit we don't then expect it to be redefined in another. Same thing for includes maybe one include is preferred over another in one unit and maybe the opposite in another unit. Unity might be faster for total rebuilds or clean builds but for every day development the compiler can usually just recompile affected units.

@vszakats
Copy link
Member Author

vszakats commented May 10, 2023

I think the practice of using unique identifiers across the whole project has its own benefits and generally improves code quality. This applies to curl as well for the necessary updates so far. As for is it worth it, like everything, this has also opponents. I'm personally in favour. curl is built more often as a whole, than incrementally. The optimization benefits can also be considerable, though I'm not yet at the point to make measurements. For sure it makes full builds almost instantaneous (except the still lenghty configuration phase.)

Regarding bugs introduced: CI tests will tell us more once I got around adding one. But first this needs to work in more build combos than a default TLS one.

@vszakats vszakats marked this pull request as draft May 10, 2023 11:50
vszakats added a commit to vszakats/curl that referenced this pull request May 10, 2023
per target as a single C source.

Can be enabled by passing `-DCMAKE_UNITY_BUILD=ON` to cmake.

It makes builds (much) faster and allows for better optimization.

Fix related issues:

- add missing include guard to `easy_lock.h`.

- rename static variables and functions (and a macro) with names reused
  across sources, or shadowed by local variables.

- add an `#undef` after use.

- add a missing `#undef` before use.

- move internal definitions from `ftp.h` to `ftp.c`.

Ref: libssh2/libssh2#1034
Ref: https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD.html
Ref: https://en.wikipedia.org/wiki/Unity_build

Closes curl#11095
@MarcelRaad
Copy link
Member

FWIW, looking at the first commit, I like the fixes independently of the question if we want to maintain this option or not. Maybe extract them to a separate PR, or at least commit, once they're done.

I find some of the changes in the second commit harder to understand when mixed with other things, like the change from struct ssl_backend_data *backend to void *backend. Maybe even make multiple commits for them to explain them in the commit message and to make them easier to review.

@vszakats
Copy link
Member Author

vszakats commented May 10, 2023

@MarcelRaad: The only "unity"-specific change is the single new line in CMakeLists.txt. Even that is optional, and only used when unity mode is explicitly enabled with cmake. Rest of the changes is all about avoiding conflicts between source files, and managing PP tricks. (There is probably further opportunities to make names more consistent after these changes in the H1-H2-proxy sources. Changes here so far are limited to what was strictly necessary.)

ssl_backend_data is the name of the struct used in each TLS backend module for the TLS context. These need to be renamed to be unique otherwise they conflict. After renaming them (as done already for OpenSSL and Schannel), struct ssl_backend_data *backend no longer finds this struct under this shared name. I changed that to a void * and added casts for accesses. It's possible there is a superior/simpler/shorter solution. On the first round my goal is to make it build at all. I'm still learning how all these bits are fitting together (esp. in MultiSSL mode).

The next riddle is [UPDATE: might be the case of selective declarations inside warnless.h, depending on BUILDING_WARNLESS_C)]:

lib/warnless.c:370:1: warning: all paths through this function will call itself [-Winfinite-recursion]
{
^
lib/warnless.c:375:1: warning: all paths through this function will call itself [-Winfinite-recursion]
{
^
2 warnings generated.

@eli-schwartz
Copy link
Contributor

The optimization benefits can also be considerable, though I'm not yet at the point to make measurements.

This is usually not meaningful compared to simply using LTO.

@bagder
Copy link
Member

bagder commented May 16, 2023

This is usually not meaningful compared to simply using LTO.

I suppose this PR can be used to test that exact claim...

vszakats added a commit to vszakats/curl that referenced this pull request May 19, 2023
per target as a single C source.

Can be enabled by passing `-DCMAKE_UNITY_BUILD=ON` to cmake.

It makes builds (much) faster and allows for better optimization.

Fix related issues:

- add missing include guard to `easy_lock.h`.

- rename static variables and functions (and a macro) with names reused
  across sources, or shadowed by local variables.

- add an `#undef` after use.

- add a missing `#undef` before use.

- move internal definitions from `ftp.h` to `ftp.c`.

Ref: libssh2/libssh2#1034
Ref: https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD.html
Ref: https://en.wikipedia.org/wiki/Unity_build

Closes curl#11095
@vszakats
Copy link
Member Author

vszakats commented May 19, 2023

LTO: IME it makes builds slower and also fragile, especially with static builds (where any dependency issue will also be hit).

Remaining issues:

  • write():
/my/curl/lib/vquic/curl_ngtcp2.c:292:43: warning: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long long') to 'unsigned int' [-Wshorten-64-to-32]
    ssize_t rc = write(ctx->qlogfd, data, datalen);
                 ~~~~~                    ^~~~~~~
1 warning generated.

Fixed two unrelated Windows issues with missing/misplaced externs.

Also had to add a tweak to CMake, to avoid compiling and linking CURLX sources twice when building a static curl tool.

vszakats added a commit to vszakats/curl that referenced this pull request May 20, 2023
per target as a single C source.

Can be enabled by passing `-DCMAKE_UNITY_BUILD=ON` to cmake.

It makes builds (much) faster and allows for better optimization.

Fix related issues:

- add missing include guard to `easy_lock.h`.

- rename static variables and functions (and a macro) with names reused
  across sources, or shadowed by local variables.

- add an `#undef` after use.

- add a missing `#undef` before use.

- move internal definitions from `ftp.h` to `ftp.c`.

Ref: libssh2/libssh2#1034
Ref: https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD.html
Ref: https://en.wikipedia.org/wiki/Unity_build

Closes curl#11095
@vszakats vszakats force-pushed the cmake-unity branch 2 times, most recently from 370b5cc to 6bb24dd Compare May 25, 2023 00:26
vszakats added a commit to vszakats/curl that referenced this pull request May 25, 2023
per target as a single C source.

Can be enabled by passing `-DCMAKE_UNITY_BUILD=ON` to cmake.

It makes builds (much) faster and allows for better optimization.

Fix related issues:

- add missing include guard to `easy_lock.h`.

- rename static variables and functions (and a macro) with names reused
  across sources, or shadowed by local variables.

- add an `#undef` after use.

- add a missing `#undef` before use.

- move internal definitions from `ftp.h` to `ftp.c`.

Ref: libssh2/libssh2#1034
Ref: https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD.html
Ref: https://en.wikipedia.org/wiki/Unity_build

Closes curl#11095
vszakats added a commit to vszakats/curl that referenced this pull request May 25, 2023
per target as a single C source.

Can be enabled by passing `-DCMAKE_UNITY_BUILD=ON` to cmake.

It makes builds (much) faster and allows for better optimization.

Fix related issues:

- add missing include guard to `easy_lock.h`.

- rename static variables and functions (and a macro) with names reused
  across sources, or shadowed by local variables.

- add an `#undef` after use.

- add a missing `#undef` before use.

- move internal definitions from `ftp.h` to `ftp.c`.

Ref: libssh2/libssh2#1034
Ref: https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD.html
Ref: https://en.wikipedia.org/wiki/Unity_build

Closes curl#11095
vszakats added a commit to vszakats/curl that referenced this pull request May 26, 2023
per target as a single C source.

Can be enabled by passing `-DCMAKE_UNITY_BUILD=ON` to cmake.

It makes builds (much) faster and allows for better optimization.

Fix related issues:

- add missing include guard to `easy_lock.h`.

- rename static variables and functions (and a macro) with names reused
  across sources, or shadowed by local variables.

- add an `#undef` after use.

- add a missing `#undef` before use.

- move internal definitions from `ftp.h` to `ftp.c`.

Ref: libssh2/libssh2#1034
Ref: https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD.html
Ref: https://en.wikipedia.org/wiki/Unity_build

Closes curl#11095
@vszakats vszakats marked this pull request as ready for review May 27, 2023 20:21
vszakats added a commit to vszakats/curl that referenced this pull request Jun 4, 2023
Sources did not use it. Autotools used it when checking for the
`winldap` library, which is redundant.

With CMake, detection was broken:
```
Run Build Command(s):/usr/local/Cellar/cmake/3.26.3/bin/cmake -E env VERBOSE=1 /usr/bin/make -f Makefile cmTC_2d8fe/fast && /Library/Developer/CommandLineTools/usr/bin/make  -f CMakeFiles/cmTC_2d8fe.dir/build.make CMakeFiles/cmTC_2d8fe.dir/build
Building C object CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj
/usr/local/opt/llvm/bin/clang --target=x86_64-w64-mingw32 --sysroot=/usr/local/opt/mingw-w64/toolchain-x86_64 -D_WINSOCKAPI_="" -I/my/quictls/x64-ucrt/usr/include -I/my/zlib/x64-ucrt/usr/include -I/my/brotli/x64-ucrt/usr/include -Wno-unused-command-line-argument   -D_UCRT -DCURL_HIDDEN_SYMBOLS -DHAVE_SSL_SET0_WBIO -DHAS_ALPN -DNGHTTP2_STATICLIB -DNGHTTP3_STATICLIB -DNGTCP2_STATICLIB -DUSE_MANUAL=1  -fuse-ld=lld -Wl,-s -static-libgcc  -lucrt  -Wextra -Wall -pedantic -Wbad-function-cast -Wconversion -Winline -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long -Wno-multichar -Wpointer-arith -Wshadow -Wsign-compare -Wundef -Wunused -Wwrite-strings -Wcast-align -Wdeclaration-after-statement -Wempty-body -Wendif-labels -Wfloat-equal -Wignored-qualifiers -Wno-format-nonliteral -Wno-sign-conversion -Wno-system-headers -Wstrict-prototypes -Wtype-limits -Wvla -Wshift-sign-overflow -Wshorten-64-to-32 -Wdouble-promotion -Wenum-conversion -Wunused-const-variable -Wcomma -Wmissing-variable-declarations -Wassign-enum -Wextra-semi-stmt  -MD -MT CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj -MF CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj.d -o CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj -c /my/curl/bld-cmake-llvm-x64-shared/CMakeFiles/CMakeScratch/TryCompile-3JP6dR/HAVE_WINLDAP_H.c
In file included from /my/curl/bld-cmake-llvm-x64-shared/CMakeFiles/CMakeScratch/TryCompile-3JP6dR/HAVE_WINLDAP_H.c:2:
In file included from /usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/winldap.h:17:
In file included from /usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/schnlsp.h:9:
In file included from /usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/schannel.h:10:
/usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/wincrypt.h:5041:254: error: unknown type name 'PSYSTEMTIME'
  WINIMPM PCCERT_CONTEXT WINAPI CertCreateSelfSignCertificate (HCRYPTPROV_OR_NCRYPT_KEY_HANDLE hCryptProvOrNCryptKey, PCERT_NAME_BLOB pSubjectIssuerBlob, DWORD dwFlags, PCRYPT_KEY_PROV_INFO pKeyProvInfo, PCRYPT_ALGORITHM_IDENTIFIER pSignatureAlgorithm, PSYSTEMTIME pStartTime, PSYSTEMTIME pEndTime, PCERT_EXTENSIONS pExtensions);
                                                                                                                                                                                                                                                             ^
/usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/wincrypt.h:5041:278: error: unknown type name 'PSYSTEMTIME'
  WINIMPM PCCERT_CONTEXT WINAPI CertCreateSelfSignCertificate (HCRYPTPROV_OR_NCRYPT_KEY_HANDLE hCryptProvOrNCryptKey, PCERT_NAME_BLOB pSubjectIssuerBlob, DWORD dwFlags, PCRYPT_KEY_PROV_INFO pKeyProvInfo, PCRYPT_ALGORITHM_IDENTIFIER pSignatureAlgorithm, PSYSTEMTIME pStartTime, PSYSTEMTIME pEndTime, PCERT_EXTENSIONS pExtensions);
                                                                                                                                                                                                                                                                                     ^
2 errors generated.
make[1]: *** [CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj] Error 1
make: *** [cmTC_2d8fe/fast] Error 2
exitCode: 2
```

Cherry-picked from curl#11095
Closes #xxxxx
vszakats added a commit to vszakats/curl that referenced this pull request Jun 4, 2023
Sources did not use it. Autotools used it when checking for the
`winldap` library, which is redundant.

With CMake, detection was broken:
```
Run Build Command(s):/usr/local/Cellar/cmake/3.26.3/bin/cmake -E env VERBOSE=1 /usr/bin/make -f Makefile cmTC_2d8fe/fast && /Library/Developer/CommandLineTools/usr/bin/make  -f CMakeFiles/cmTC_2d8fe.dir/build.make CMakeFiles/cmTC_2d8fe.dir/build
Building C object CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj
/usr/local/opt/llvm/bin/clang --target=x86_64-w64-mingw32 --sysroot=/usr/local/opt/mingw-w64/toolchain-x86_64 -D_WINSOCKAPI_="" -I/my/quictls/x64-ucrt/usr/include -I/my/zlib/x64-ucrt/usr/include -I/my/brotli/x64-ucrt/usr/include -Wno-unused-command-line-argument   -D_UCRT -DCURL_HIDDEN_SYMBOLS -DHAVE_SSL_SET0_WBIO -DHAS_ALPN -DNGHTTP2_STATICLIB -DNGHTTP3_STATICLIB -DNGTCP2_STATICLIB -DUSE_MANUAL=1  -fuse-ld=lld -Wl,-s -static-libgcc  -lucrt  -Wextra -Wall -pedantic -Wbad-function-cast -Wconversion -Winline -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long -Wno-multichar -Wpointer-arith -Wshadow -Wsign-compare -Wundef -Wunused -Wwrite-strings -Wcast-align -Wdeclaration-after-statement -Wempty-body -Wendif-labels -Wfloat-equal -Wignored-qualifiers -Wno-format-nonliteral -Wno-sign-conversion -Wno-system-headers -Wstrict-prototypes -Wtype-limits -Wvla -Wshift-sign-overflow -Wshorten-64-to-32 -Wdouble-promotion -Wenum-conversion -Wunused-const-variable -Wcomma -Wmissing-variable-declarations -Wassign-enum -Wextra-semi-stmt  -MD -MT CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj -MF CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj.d -o CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj -c /my/curl/bld-cmake-llvm-x64-shared/CMakeFiles/CMakeScratch/TryCompile-3JP6dR/HAVE_WINLDAP_H.c
In file included from /my/curl/bld-cmake-llvm-x64-shared/CMakeFiles/CMakeScratch/TryCompile-3JP6dR/HAVE_WINLDAP_H.c:2:
In file included from /usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/winldap.h:17:
In file included from /usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/schnlsp.h:9:
In file included from /usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/schannel.h:10:
/usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/wincrypt.h:5041:254: error: unknown type name 'PSYSTEMTIME'
  WINIMPM PCCERT_CONTEXT WINAPI CertCreateSelfSignCertificate (HCRYPTPROV_OR_NCRYPT_KEY_HANDLE hCryptProvOrNCryptKey, PCERT_NAME_BLOB pSubjectIssuerBlob, DWORD dwFlags, PCRYPT_KEY_PROV_INFO pKeyProvInfo, PCRYPT_ALGORITHM_IDENTIFIER pSignatureAlgorithm, PSYSTEMTIME pStartTime, PSYSTEMTIME pEndTime, PCERT_EXTENSIONS pExtensions);
                                                                                                                                                                                                                                                             ^
/usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/wincrypt.h:5041:278: error: unknown type name 'PSYSTEMTIME'
  WINIMPM PCCERT_CONTEXT WINAPI CertCreateSelfSignCertificate (HCRYPTPROV_OR_NCRYPT_KEY_HANDLE hCryptProvOrNCryptKey, PCERT_NAME_BLOB pSubjectIssuerBlob, DWORD dwFlags, PCRYPT_KEY_PROV_INFO pKeyProvInfo, PCRYPT_ALGORITHM_IDENTIFIER pSignatureAlgorithm, PSYSTEMTIME pStartTime, PSYSTEMTIME pEndTime, PCERT_EXTENSIONS pExtensions);
                                                                                                                                                                                                                                                                                     ^
2 errors generated.
make[1]: *** [CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj] Error 1
make: *** [cmTC_2d8fe/fast] Error 2
exitCode: 2
```

Cherry-picked from curl#11095 88e4a21
Closes #xxxxx
```
C:/projects/curl/lib/hsts.c(60,15): error C2084: function 'time_t debugtime(void *)' already has a body [C:\projects\curl\lib\libcurl.vcxproj]
C:/projects/curl/lib/altsvc.c(427,15): message : see previous definition of 'debugtime' [C:\projects\curl\lib\libcurl.vcxproj]
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/47218733/job/fxt21ujjnckls1dc#L237

```
In file included from C:/projects/curl/lib/CMakeFiles/libcurl.dir/Unity/unity_0.c:64:0:
C:/projects/curl/lib/hsts.c:60:15: error: redefinition of 'debugtime'
 static time_t debugtime(void *unused)
               ^~~~~~~~~
In file included from C:/projects/curl/lib/CMakeFiles/libcurl.dir/Unity/unity_0.c:3:0:
C:/projects/curl/lib/altsvc.c:427:15: note: previous definition of 'debugtime' was here
 static time_t debugtime(void *unused)
               ^~~~~~~~~
In file included from C:/projects/curl/lib/CMakeFiles/libcurl.dir/Unity/unity_0.c:110:0:
C:/projects/curl/lib/socks.c: In function 'socksstate':
C:/projects/curl/lib/socks.c:164:29: error: declaration of 'statename' shadows a global declaration [-Werror=shadow]
   static const char * const statename[] = {
                             ^~~~~~~~~
In file included from C:/projects/curl/lib/CMakeFiles/libcurl.dir/Unity/unity_0.c:88:0:
C:/projects/curl/lib/multi.c:115:27: note: shadowed declaration is here
 static const char * const statename[]={
                           ^~~~~~~~~
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/47218733/job/htg2pm8276olu0vp#L245
Move memdebug.c into a separate unity group, forcing it to
compile separately and without going into recursive PP rules
when debug mode is enabled.
A unity fallout happens with openssl + schannel builds.
disabling openssl fixes it:
```
C:/projects/curl/lib/vtls/schannel.c: In function 'schannel_connect_step1':
C:/projects/curl/lib/vtls/schannel.c:1217:7: error: 'SecApplicationProtocolNegotiationExt_ALPN' undeclared (first use in this function)
       SecApplicationProtocolNegotiationExt_ALPN;
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/projects/curl/lib/vtls/schannel.c:1217:7: note: each undeclared identifier is reported only once for each function it appears in
C:/projects/curl/lib/vtls/schannel.c:1240:27: error: 'SECBUFFER_APPLICATION_PROTOCOLS' undeclared (first use in this function); did you mean 'SECBUFFER_NEGOTIATION_INFO'?
     InitSecBuffer(&inbuf, SECBUFFER_APPLICATION_PROTOCOLS, alpn_buffer, cur);
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                           SECBUFFER_NEGOTIATION_INFO
C:/projects/curl/lib/vtls/schannel.c: In function 'schannel_connect_step3':
C:/projects/curl/lib/vtls/schannel.c:1698:3: error: unknown type name 'SecPkgContext_ApplicationProtocol'; did you mean 'SecPkgContext_ConnectionInfo'?
   SecPkgContext_ApplicationProtocol alpn_result;
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   SecPkgContext_ConnectionInfo
C:/projects/curl/lib/vtls/schannel.c:1730:40: error: 'SECPKG_ATTR_APPLICATION_PROTOCOL' undeclared (first use in this function); did you mean 'SECPKG_ATTR_SUPPORTED_PROTOCOLS'?
                                        SECPKG_ATTR_APPLICATION_PROTOCOL,
                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                        SECPKG_ATTR_SUPPORTED_PROTOCOLS
C:/projects/curl/lib/vtls/schannel.c:1738:19: error: request for member 'ProtoNegoStatus' in something not a structure or union
     if(alpn_result.ProtoNegoStatus ==
                   ^
C:/projects/curl/lib/vtls/schannel.c:1739:8: error: 'SecApplicationProtocolNegotiationStatus_Success' undeclared (first use in this function)
        SecApplicationProtocolNegotiationStatus_Success) {
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/projects/curl/lib/vtls/schannel.c:1742:53: error: request for member 'ProtocolId' in something not a structure or union
       Curl_alpn_set_negotiated(cf, data, alpn_result.ProtocolId,
                                                     ^
C:/projects/curl/lib/vtls/schannel.c:1743:43: error: request for member 'ProtocolIdSize' in something not a structure or union
                                alpn_result.ProtocolIdSize);
                                           ^
make[2]: *** [lib/CMakeFiles/libcurl.dir/build.make:64: lib/CMakeFiles/libcurl.dir/Unity/unity_0.c.obj] Error 1
make[2]: Target 'lib/CMakeFiles/libcurl.dir/build' not remade because of errors
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/47230972/job/51wfesgnfuauwl8q#L250

Bad attempts:
https://ci.appveyor.com/project/curlorg/curl/builds/47230369
https://ci.appveyor.com/project/curlorg/curl/builds/47230369/job/cpr9b67a2al293qj
  `tests\unit\unit3200.exe` -> hung
https://ci.appveyor.com/project/curlorg/curl/builds/47230369/job/6usv7td7sfb6f4bm
  `could not get curl version! at C:/projects/curl/tests/runtests.pl line 2360.`

Also:
- appveyor.yml: add 'dll' to libcurl artifact name
@vszakats
Copy link
Member Author

vszakats commented Jun 6, 2023

One AppVeyor CI job converted to unity, one other added. Both Windows,
the notorious problem-platform, so it will be enough for a start.

Caveats:

  • Running tests doesn't work in all cases. This will be an exercise for the future.
  • One random build combo (the new AppVeyor CI one with OpenSSL enabled
    alongside Schannel) produced yet another bizarre Windows Schannel PP
    fallout. There is probably more of these, especially on Windows, which is the
    hot mess it usually is. This can be addressed as they come along in real life
    or as interest / free time arises.

At this point I deem this mergable.

@vszakats vszakats closed this in 3f8fc25 Jun 7, 2023
@vszakats vszakats deleted the cmake-unity branch June 7, 2023 13:09
@eli-schwartz
Copy link
Contributor

I remain generally skeptical of the values of unity builds, and maintain that if you want better optimization you should be building with LTO.

LTO: IME it makes builds slower and also fragile, especially with static builds (where any dependency issue will also be hit).

So LTO is the fragile thing, despite that this PR introduces "bizarre" fallout and generally has issues running tests? It seems less than ideal to be introducing a compile mode that isn't being thoroughly tested.

I'm curious what the apparent fragility of LTO is. I mean, yes, LTO tends to produce issues where existing UB gets miscompiled with LTO and has benevolent effects without LTO, but that's really no different in concept from building with higher -O settings, and the solution is to always fix the codebase in question to stop doing UB.

I'm also not sure what static builds or dependencies has to do with anything. No one suggested doing a unity build that gathers all dependencies into a single translation unit together with the curl sources -- by the same extension no one is under any obligation to use LTO across both curl and its static dependencies. Although the good news is that unlike unity builds (which will definitely not work with completely external project sources!) LTO is capable of doing that kind of optimization.

Be that as it may, I can happily report that curl builds fine with LTO on my machine (and passes all tests). And it doesn't even need a few thousand lines of changes that incur an ongoing maintenance cost which isn't revealed in the default build mode. :)

@thesamesam
Copy link
Contributor

In addition to what you've said (and I wholeheartedly agree), unity builds in this form introduce variance across the build systems. And even with all of this, tests had to be skipped and weren't solved yet.

Meanwhile, many Linux distributions are building with LTO by default - including building curl, with no complaints.

@vszakats
Copy link
Member Author

vszakats commented Jun 7, 2023

This PR doesn't disable the option to build curl with LTO. Nor is this against LTO, or in fact much if anything to do with LTO. It adds unity as a build option for those who might find it useful. It also unearthed some code smells.

[ On Windows (and MinGW), LTO makes build times considerably higher. When combined with static linking you're facing all the LTO bugs of all dependencies, which is sometimes more than one is willing (or able) to fix. LTO objects were (likely still are) compiler version specific with GCC, and had to deal with the fallout after each compiler update. It's a useful feature, but not necessarily for every use-case. ]

Bizarre fallouts are pretty common on Windows, unity builds and LTO aside.

@eli-schwartz
Copy link
Contributor

I'm still confused by this "all the LTO bugs of all dependencies" thing, unless you just mean "C projects always have UB"?

I also don't think you really have to worry about compiler versions. Distros tend to support only a single version, and rebuild their entire stack as needed to ensure their packages are compatible (this is really a static libraries concern though), and outside of distros, people tend to stick to a single compiler version in their project anyway -- because of concerns about C++ compatibility for example.

...

While it's true that LTO can result in increased build times, I think it is generally meritorious. Someone who is seeking ways to increase runtime performance I would assume considers that to be a valuable priority, well worth spending a bit more time compiling releases. For optimizing the development cycle, one would simply turn off LTO because speed of testing loses out to speed of iterative testing... and for the same reason, one would turn off unity during development, because unity builds have a significant negative effect on your incremental build times. Even with a relatively low chunk size, the more you iterate the more you pay the cost for compiling multiple times the amount of code you modified.

So honestly, the motivation for adding support seems weak. And yes, I do understand that this doesn't prevent using LTO anyway. That was never my concern and if I accidentally gave that impression I apologize (although I'm not actually sure how I would have given that impression).

Again: my concerns are that

  • it produces cases that are known to be "bizarre", and claiming that it's common to have bizarre issues on Windows seems a poor excuse for merging what is apparently unfinished work,
  • it's acknowledged that the result of this is builds that can't be tested, and is thus apparently unfinished work, and
  • unity builds in general mean that one is committing to an ongoing maintenance burden of keeping it working, adding additional testing matrixes and then dealing with the fallout of people who successfully tested their code contributions the normal way and then had it fail in CI with obscure errors, because it's now necessary to avoid perfectly reasonable and idiomatic, correct code, because it misbehaves when scoping rules are violated and different translation units see each others' headers and macros and trample all over private local symbols

Maybe I'd feel less weird about it if it was obvious to me that the curl maintainers in general feel it's worth the maintenance burden. That concern was raised above -- it was the first response -- and then not brought up again.

Maybe I'd feel less weird about it if it didn't include big disclaimers about how the PR is incomplete and on the single biggest platform, it has "bizarre" issues -- and those issues appear to be exactly an example of my concern that unity is in fact fragile, flaky and buggy. It gives the impression that curl is okay merging and officially supporting buggy configurations, including configurations that build with a passing return code but are suspect of incorrect runtime behavior.

And this PR is a lot of work to do for something that has such a glaring issue.

@vszakats
Copy link
Member Author

vszakats commented Jun 7, 2023

I believe one main difference is that I tried making LTO work on Windows (in the distant past), where things were/are much bumpier than in a Linux env, whereas you seem to have done the same on Linux, where it's most probably just a linker switch away.

This PR is already done, out of my free time. As I said bizarre issues are business as usual on Windows, and all of them here were Windows related. You're welcome to pick up any of the issues and reflect to that, but skimming through weeks of work, picking out 2-3 uncomfortable words in comments, don't seem to be a productive take on this.

Anyhow, I suggest opening a discussion of the merits of LTO. I'm not personally interested in it, and this PR has nothing to do with it.

bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
Sources did not use it. Autotools used it when checking for the
`winldap` library, which is redundant.

With CMake, detection was broken:
```
Run Build Command(s):/usr/local/Cellar/cmake/3.26.3/bin/cmake -E env VERBOSE=1 /usr/bin/make -f Makefile cmTC_2d8fe/fast && /Library/Developer/CommandLineTools/usr/bin/make  -f CMakeFiles/cmTC_2d8fe.dir/build.make CMakeFiles/cmTC_2d8fe.dir/build
Building C object CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj
/usr/local/opt/llvm/bin/clang --target=x86_64-w64-mingw32 --sysroot=/usr/local/opt/mingw-w64/toolchain-x86_64 -D_WINSOCKAPI_="" -I/my/quictls/x64-ucrt/usr/include -I/my/zlib/x64-ucrt/usr/include -I/my/brotli/x64-ucrt/usr/include -Wno-unused-command-line-argument   -D_UCRT -DCURL_HIDDEN_SYMBOLS -DHAVE_SSL_SET0_WBIO -DHAS_ALPN -DNGHTTP2_STATICLIB -DNGHTTP3_STATICLIB -DNGTCP2_STATICLIB -DUSE_MANUAL=1  -fuse-ld=lld -Wl,-s -static-libgcc  -lucrt  -Wextra -Wall -pedantic -Wbad-function-cast -Wconversion -Winline -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long -Wno-multichar -Wpointer-arith -Wshadow -Wsign-compare -Wundef -Wunused -Wwrite-strings -Wcast-align -Wdeclaration-after-statement -Wempty-body -Wendif-labels -Wfloat-equal -Wignored-qualifiers -Wno-format-nonliteral -Wno-sign-conversion -Wno-system-headers -Wstrict-prototypes -Wtype-limits -Wvla -Wshift-sign-overflow -Wshorten-64-to-32 -Wdouble-promotion -Wenum-conversion -Wunused-const-variable -Wcomma -Wmissing-variable-declarations -Wassign-enum -Wextra-semi-stmt  -MD -MT CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj -MF CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj.d -o CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj -c /my/curl/bld-cmake-llvm-x64-shared/CMakeFiles/CMakeScratch/TryCompile-3JP6dR/HAVE_WINLDAP_H.c
In file included from /my/curl/bld-cmake-llvm-x64-shared/CMakeFiles/CMakeScratch/TryCompile-3JP6dR/HAVE_WINLDAP_H.c:2:
In file included from /usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/winldap.h:17:
In file included from /usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/schnlsp.h:9:
In file included from /usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/schannel.h:10:
/usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/wincrypt.h:5041:254: error: unknown type name 'PSYSTEMTIME'
  WINIMPM PCCERT_CONTEXT WINAPI CertCreateSelfSignCertificate (HCRYPTPROV_OR_NCRYPT_KEY_HANDLE hCryptProvOrNCryptKey, PCERT_NAME_BLOB pSubjectIssuerBlob, DWORD dwFlags, PCRYPT_KEY_PROV_INFO pKeyProvInfo, PCRYPT_ALGORITHM_IDENTIFIER pSignatureAlgorithm, PSYSTEMTIME pStartTime, PSYSTEMTIME pEndTime, PCERT_EXTENSIONS pExtensions);
                                                                                                                                                                                                                                                             ^
/usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/wincrypt.h:5041:278: error: unknown type name 'PSYSTEMTIME'
  WINIMPM PCCERT_CONTEXT WINAPI CertCreateSelfSignCertificate (HCRYPTPROV_OR_NCRYPT_KEY_HANDLE hCryptProvOrNCryptKey, PCERT_NAME_BLOB pSubjectIssuerBlob, DWORD dwFlags, PCRYPT_KEY_PROV_INFO pKeyProvInfo, PCRYPT_ALGORITHM_IDENTIFIER pSignatureAlgorithm, PSYSTEMTIME pStartTime, PSYSTEMTIME pEndTime, PCERT_EXTENSIONS pExtensions);
                                                                                                                                                                                                                                                                                     ^
2 errors generated.
make[1]: *** [CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj] Error 1
make: *** [cmTC_2d8fe/fast] Error 2
exitCode: 2
```

Cherry-picked from curl#11095 88e4a21
Reviewed-by: Daniel Stenberg
Closes curl#11245
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
Aka "jumbo" or "amalgamation" builds. It means to compile all sources
per target as a single C source. This is experimental.

You can enable it by passing `-DCMAKE_UNITY_BUILD=ON` to cmake.
It requires CMake 3.16 or newer.

It makes builds (much) faster, allows for better optimizations and tends
to promote less ambiguous code.

Also add a new AppVeyor CI job and convert an existing one to use
"unity" mode (one MSVC, one MinGW), and enable it for one macOS CI job.

Fix related issues:
- add missing include guard to `easy_lock.h`.
- rename static variables and functions (and a macro) with names reused
  across sources, or shadowed by local variables.
- add an `#undef` after use.
- add a missing `#undef` before use.
- move internal definitions from `ftp.h` to `ftp.c`.
- `curl_memory.h` fixes to make it work when included repeatedly.
- stop building/linking curlx bits twice for a static-mode curl tool.
  These caused doubly defined symbols in unity builds.
- silence missing extern declarations compiler warning for ` _CRT_glob`.
- fix extern declarations for `tool_freq` and `tool_isVistaOrGreater`.
- fix colliding static symbols in debug mode: `debugtime()` and
  `statename`.
- rename `ssl_backend_data` structure to unique names for each
  TLS-backend, along with the `ssl_connect_data` struct member
  referencing them. This required adding casts for each access.
- add workaround for missing `[P]UNICODE_STRING` types in certain Windows
  builds when compiling `lib/ldap.c`. To support "unity" builds, we had
  to enable `SCHANNEL_USE_BLACKLISTS` for Schannel (a Windows
  `schannel.h` option) _globally_. This caused an indirect inclusion of
  Windows `schannel.h` from `ldap.c` via `winldap.h` to have it enabled
  as well. This requires `[P]UNICODE_STRING` types, which is apperantly
  not defined automatically (as seen with both MSVS and mingw-w64).
  This patch includes `<subauth.h>` to fix it.
  Ref: https://github.com/curl/curl/runs/13987772013
  Ref: https://dev.azure.com/daniel0244/curl/_build/results?buildId=15827&view=logs&jobId=2c9f582d-e278-56b6-4354-f38a4d851906&j=2c9f582d-e278-56b6-4354-f38a4d851906&t=90509b00-34fa-5a81-35d7-5ed9569d331c
- tweak unity builds to compile `lib/memdebug.c` separately in memory
  trace builds to avoid PP confusion.
- force-disable unity for test programs.
- do not compile and link libcurl sources to libtests _twice_ when libcurl
  is built in static mode.

KNOWN ISSUES:
- running tests with unity builds may fail in cases.
- some build configurations/env may not compile in unity mode. E.g.:
  https://ci.appveyor.com/project/curlorg/curl/builds/47230972/job/51wfesgnfuauwl8q#L250

Ref: libssh2/libssh2#1034
Ref: https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD.html
Ref: https://en.wikipedia.org/wiki/Unity_build

Closes curl#11095
@AndrewJDR
Copy link

AndrewJDR commented Aug 23, 2023

@vszakats Thanks for all the effort on this. I'm very sad that this is closed!

For me, what is truly exciting about a working unity / amalgamated libcurl is that it gets libcurl one step closer to this (from a twitter thread):

preconfigured and amalgamated source packages (e.g. "here's a single-source-libcurl preconfigured for Win32 and HTTP to drop right into your project")

@vszakats Do you think this sort of thing would be possible? Basically, if you're trying to ship a project that includes libcurl as source with minimal dependencies -- i.e. without requiring those adopting it to have cmake, etc), one could include a few pre-generated .c files, one for each platform that your project wants to support.

For example, someone supporting windows and linux might have these two files in in their project's repository:
deps/libcurl_win32_schannel.c
deps/libcurl_linux_mbedtls.c

Then, a simple build.bat and build.sh file could be used for the project, rather than burdening the developer with dealing with complex build systems like cmake. This would be a huge simplification for projects where libcurl is the only "complex" dependency that requires a build system.

* Obviously, for the deps/libcurl_linux_mbedtls.c example, mbedtls would need to be included in the repository as well, but mbedtls is actually fairly trivial to ship along with the repo without a build system. Plus that's outside the scope here, which is just getting libcurl to support unity builds.

@vszakats
Copy link
Member Author

@AndrewJDR We always merge manually in curl, leaving PRs show up as "closed". I did merge this, so the feature is live. It's not as extensively tested by the CI as I'd want to though, and recently fixed (#11095) a regression in H2 builds.

What you say is technically possible after the efforts made in this PR, but curl for now leans on CMake to do the "amalgamation" part dynamically, as part of its build. Meaning we have no separate script that converts libcurl sources to a single .c file. CMake internally creates a wrapper .c file that #includes libcurl C sources, so it's not as easy to copy off an amalgamated .c from its build tree as-is either. But, certainly doable with some script foo. Haven't tried, but even a cat command might actually work.

@AndrewJDR
Copy link

@vszakats Wow that's great! I will definitely give it a try some time.

ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
Sources did not use it. Autotools used it when checking for the
`winldap` library, which is redundant.

With CMake, detection was broken:
```
Run Build Command(s):/usr/local/Cellar/cmake/3.26.3/bin/cmake -E env VERBOSE=1 /usr/bin/make -f Makefile cmTC_2d8fe/fast && /Library/Developer/CommandLineTools/usr/bin/make  -f CMakeFiles/cmTC_2d8fe.dir/build.make CMakeFiles/cmTC_2d8fe.dir/build
Building C object CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj
/usr/local/opt/llvm/bin/clang --target=x86_64-w64-mingw32 --sysroot=/usr/local/opt/mingw-w64/toolchain-x86_64 -D_WINSOCKAPI_="" -I/my/quictls/x64-ucrt/usr/include -I/my/zlib/x64-ucrt/usr/include -I/my/brotli/x64-ucrt/usr/include -Wno-unused-command-line-argument   -D_UCRT -DCURL_HIDDEN_SYMBOLS -DHAVE_SSL_SET0_WBIO -DHAS_ALPN -DNGHTTP2_STATICLIB -DNGHTTP3_STATICLIB -DNGTCP2_STATICLIB -DUSE_MANUAL=1  -fuse-ld=lld -Wl,-s -static-libgcc  -lucrt  -Wextra -Wall -pedantic -Wbad-function-cast -Wconversion -Winline -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long -Wno-multichar -Wpointer-arith -Wshadow -Wsign-compare -Wundef -Wunused -Wwrite-strings -Wcast-align -Wdeclaration-after-statement -Wempty-body -Wendif-labels -Wfloat-equal -Wignored-qualifiers -Wno-format-nonliteral -Wno-sign-conversion -Wno-system-headers -Wstrict-prototypes -Wtype-limits -Wvla -Wshift-sign-overflow -Wshorten-64-to-32 -Wdouble-promotion -Wenum-conversion -Wunused-const-variable -Wcomma -Wmissing-variable-declarations -Wassign-enum -Wextra-semi-stmt  -MD -MT CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj -MF CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj.d -o CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj -c /my/curl/bld-cmake-llvm-x64-shared/CMakeFiles/CMakeScratch/TryCompile-3JP6dR/HAVE_WINLDAP_H.c
In file included from /my/curl/bld-cmake-llvm-x64-shared/CMakeFiles/CMakeScratch/TryCompile-3JP6dR/HAVE_WINLDAP_H.c:2:
In file included from /usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/winldap.h:17:
In file included from /usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/schnlsp.h:9:
In file included from /usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/schannel.h:10:
/usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/wincrypt.h:5041:254: error: unknown type name 'PSYSTEMTIME'
  WINIMPM PCCERT_CONTEXT WINAPI CertCreateSelfSignCertificate (HCRYPTPROV_OR_NCRYPT_KEY_HANDLE hCryptProvOrNCryptKey, PCERT_NAME_BLOB pSubjectIssuerBlob, DWORD dwFlags, PCRYPT_KEY_PROV_INFO pKeyProvInfo, PCRYPT_ALGORITHM_IDENTIFIER pSignatureAlgorithm, PSYSTEMTIME pStartTime, PSYSTEMTIME pEndTime, PCERT_EXTENSIONS pExtensions);
                                                                                                                                                                                                                                                             ^
/usr/local/opt/mingw-w64/toolchain-x86_64/x86_64-w64-mingw32/include/wincrypt.h:5041:278: error: unknown type name 'PSYSTEMTIME'
  WINIMPM PCCERT_CONTEXT WINAPI CertCreateSelfSignCertificate (HCRYPTPROV_OR_NCRYPT_KEY_HANDLE hCryptProvOrNCryptKey, PCERT_NAME_BLOB pSubjectIssuerBlob, DWORD dwFlags, PCRYPT_KEY_PROV_INFO pKeyProvInfo, PCRYPT_ALGORITHM_IDENTIFIER pSignatureAlgorithm, PSYSTEMTIME pStartTime, PSYSTEMTIME pEndTime, PCERT_EXTENSIONS pExtensions);
                                                                                                                                                                                                                                                                                     ^
2 errors generated.
make[1]: *** [CMakeFiles/cmTC_2d8fe.dir/HAVE_WINLDAP_H.c.obj] Error 1
make: *** [cmTC_2d8fe/fast] Error 2
exitCode: 2
```

Cherry-picked from curl#11095 88e4a21
Reviewed-by: Daniel Stenberg
Closes curl#11245
ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023
Aka "jumbo" or "amalgamation" builds. It means to compile all sources
per target as a single C source. This is experimental.

You can enable it by passing `-DCMAKE_UNITY_BUILD=ON` to cmake.
It requires CMake 3.16 or newer.

It makes builds (much) faster, allows for better optimizations and tends
to promote less ambiguous code.

Also add a new AppVeyor CI job and convert an existing one to use
"unity" mode (one MSVC, one MinGW), and enable it for one macOS CI job.

Fix related issues:
- add missing include guard to `easy_lock.h`.
- rename static variables and functions (and a macro) with names reused
  across sources, or shadowed by local variables.
- add an `#undef` after use.
- add a missing `#undef` before use.
- move internal definitions from `ftp.h` to `ftp.c`.
- `curl_memory.h` fixes to make it work when included repeatedly.
- stop building/linking curlx bits twice for a static-mode curl tool.
  These caused doubly defined symbols in unity builds.
- silence missing extern declarations compiler warning for ` _CRT_glob`.
- fix extern declarations for `tool_freq` and `tool_isVistaOrGreater`.
- fix colliding static symbols in debug mode: `debugtime()` and
  `statename`.
- rename `ssl_backend_data` structure to unique names for each
  TLS-backend, along with the `ssl_connect_data` struct member
  referencing them. This required adding casts for each access.
- add workaround for missing `[P]UNICODE_STRING` types in certain Windows
  builds when compiling `lib/ldap.c`. To support "unity" builds, we had
  to enable `SCHANNEL_USE_BLACKLISTS` for Schannel (a Windows
  `schannel.h` option) _globally_. This caused an indirect inclusion of
  Windows `schannel.h` from `ldap.c` via `winldap.h` to have it enabled
  as well. This requires `[P]UNICODE_STRING` types, which is apperantly
  not defined automatically (as seen with both MSVS and mingw-w64).
  This patch includes `<subauth.h>` to fix it.
  Ref: https://github.com/curl/curl/runs/13987772013
  Ref: https://dev.azure.com/daniel0244/curl/_build/results?buildId=15827&view=logs&jobId=2c9f582d-e278-56b6-4354-f38a4d851906&j=2c9f582d-e278-56b6-4354-f38a4d851906&t=90509b00-34fa-5a81-35d7-5ed9569d331c
- tweak unity builds to compile `lib/memdebug.c` separately in memory
  trace builds to avoid PP confusion.
- force-disable unity for test programs.
- do not compile and link libcurl sources to libtests _twice_ when libcurl
  is built in static mode.

KNOWN ISSUES:
- running tests with unity builds may fail in cases.
- some build configurations/env may not compile in unity mode. E.g.:
  https://ci.appveyor.com/project/curlorg/curl/builds/47230972/job/51wfesgnfuauwl8q#L250

Ref: libssh2/libssh2#1034
Ref: https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD.html
Ref: https://en.wikipedia.org/wiki/Unity_build

Closes curl#11095
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
Follow-up to 3f8fc25 curl#11095

Closes curl#11928
vszakats added a commit that referenced this pull request Oct 3, 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
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
Development

Successfully merging this pull request may close these issues.

None yet

7 participants