build: untangle CURLDEBUG and DEBUGBUILD macros#13718
Closed
vszakats wants to merge 6 commits intocurl:masterfrom
Closed
build: untangle CURLDEBUG and DEBUGBUILD macros#13718vszakats wants to merge 6 commits intocurl:masterfrom
CURLDEBUG and DEBUGBUILD macros#13718vszakats wants to merge 6 commits intocurl:masterfrom
Conversation
CURLDEBUG with DEBUGBUILD for debug featuresCURLDEBUG and DEBUGBUILD macros
5d13f73 to
48803a5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
256032e to
2d61b35
Compare
fa4c9b4 to
7330e15
Compare
1b4f7ee to
26cab7e
Compare
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
May 27, 2024
Cherry-picked from curl#13718 Closes #xxxxx
vszakats
added a commit
to vszakats/curl
that referenced
this pull request
May 27, 2024
Before this patch, `ENABLE_CURLDEBUG` (memory tracking) was unconditionally enabled when `ENABLE_DEBUGBUILD` was set. This made testing some build configurations complicated. To fix this, after this patch we only enable `ENABLE_CURLDEBUG` if not already defined by the user. This allows to use this config: ENABLE_DEBUGBUILD=ON ENABLE_CURLDEBUG=OFF to enable debug features, without also enabling memory tracking. This is important because some other build methods allow to set one of these features but not the other. This patch allows to test any combination with CMake. This makes it unnecessary to use the workaround of passing `-DDEBUGBUILD` via `CMAKE_C_FLAGS`. Which has the disadvantage that our CMake logic cannot easily detect it, e.g. for disabling symbol hiding on Windows for `ENABLE_DEBUG`/`DEBUGBUILD` builds. Cherry-picked from curl#13718 Closes #xxxxx
vszakats
added a commit
that referenced
this pull request
May 27, 2024
Before this patch, `ENABLE_CURLDEBUG` (memory tracking) was unconditionally enabled when `ENABLE_DEBUGBUILD` was set. This made testing some build configurations complicated. To fix it, this patch makes `ENABLE_CURLDEBUG` to receive the value of `ENABLE_DEBUG` by default, while allowing free override by the user. This allows to use the config: `ENABLE_DEBUGBUILD=ON ENABLE_CURLDEBUG=OFF` to enable debug features, without also enabling memory tracking. This is important because some other build methods allow to set one of these features but not the other. This patch allows to test any combination with CMake. This makes it unnecessary to use the workaround of passing `-DDEBUGBUILD` via `CMAKE_C_FLAGS`. Which has the disadvantage that our CMake logic cannot easily detect it, e.g. for disabling symbol hiding on Windows for `ENABLE_DEBUG`/`DEBUGBUILD` builds. Cherry-picked from #13718 Closes #13792
vszakats
added a commit
that referenced
this pull request
May 27, 2024
```
curl/lib/http_aws_sigv4.c:536:10: error: 'clock' may be used uninitialized [-Werror=maybe-uninitialized]
536 | time_t clock;
| ^~~~~
```
Ref: https://github.com/curl/curl/actions/runs/9158755123/job/25177765000#step:13:79
Cherry-picked from #13718
Closes #13800
Instead of `CURLDEBUG`, which did work in most cases because debug builds (almost?) always enable `CURLDEBUG`, but it's not accurate. Unless this was chosen intentionally for some reason I'm missing. (E.g. to enable this for non-debug builds with `--enable-curldebug`? But this option means `Enable curl debug memory tracking`, which seems to suggest otherwise.)
To fix:
MSVS: CMake, VS2022, Debug, x64, Schannel, Shared, DEBUGBUILD=ON, CURLDEBUG=OFF
```
unity_0_c.obj : error LNK2019: unresolved external symbol curl_easy_perform_ev referenced in function serial_transfers [curl\_bld\src\curl.vcxproj]
10>curl\_bld\src\curl.exe : fatal error LNK1120: 1 unresolved externals
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/49852346/job/v71t14kc6os47s31#L261
mingw-w64: CMake, mingw-w64, gcc 13, Debug, x64, Schannel, Shared, DEBUGBUILD=ON, CURLDEBUG=OFF
```
CMakeFiles/curl.dir/objects.a(unity_0_c.c.obj): in function `serial_transfers':
1447C:/projects/curl/src/tool_operate.c:2495:(.text+0x18777): undefined reference to `curl_easy_perform_ev'
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/49852346/job/lig8hj3570knux38#L197
But, this was not enough because we're using libcurl.def for exporting
functions and curl_easy_perform_ev() is not on the list.
The reason why it worked before is because CMake has special logic
to disable symbol hiding for ENABLE_CURLDEBUG. We must now do the same
for ENABLE_DEBUGBUILD. But, that's not so simple either because the
only way to test DEBUGBUILD without CURLDEBUG, is passing -DDEBUGBUILD
via CMAKE_C_FLAGS, in which case ENABLE_DEBUGBUILD is OFF.
Ref: 6cf8413
~~Also CURL_EXTERN not necessary for curl_easy_perform_ev, since we rely
on disabling symbol hiding to export this. Just like for curl_dbg_*()
functions used for memory tracking.~~ [I misread this, CURL_EXTERN is
actually required for non-Windows/autotools systems.]
But, did this ever work for autotools / Windows builds? [It did/does,
the trick is that dll exports are handled dynamically by libtool, so
the hard-wirred libcurl.def list is not used here.]
edit: the patch fixed it: https://ci.appveyor.com/project/curlorg/curl/builds/49852747
Modify to test a fragile debug build case.
26cab7e to
274f23f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CURLDEBUGis meant to enable memory tracking, but in a bunch of cases,it was protecting debug features that were supposed to be guarded with
DEBUGBUILD.Replace these uses with
DEBUGBUILD.This leaves
CURLDEBUGuses solely for its intended purpose: to enablethe memory tracking debug feature.
Also:
DEBUGBUILDto enablechecksrc.Instead of
CURLDEBUG, which worked in most cases because debugbuilds enable
CURLDEBUGby default, but it's not accurate.lib/easyif.hinstead of keeping a copy of a declaration.Ref: #13694 (comment)
Closes #13718
This PR builds upon #13694 and also includes #13592, #13698, #13705
for testing everything related in one.
TODO:
curl_easy_perform_ev()from libcurl. This uses a workaround hard-wired toENABLE_CURLDEBUGin CMake, which needs to be updated. (Couldn't spot the same trick in autotools.) Job fallout: https://ci.appveyor.com/project/curlorg/curl/builds/49852346. Needs further fixing for autotools / non-Windows.TESTINGtoTESTBUILDandTESTRUN.TESTBUILDdefaulting toTESTRUN.