-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
build: untangle CURLDEBUG
and DEBUGBUILD
macros
#13718
Closed
Closed
Conversation
This file contains 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
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
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
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
CURLDEBUG
is 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
CURLDEBUG
uses solely for its intended purpose: to enablethe memory tracking debug feature.
Also:
DEBUGBUILD
to enablechecksrc
.Instead of
CURLDEBUG
, which worked in most cases because debugbuilds enable
CURLDEBUG
by default, but it's not accurate.lib/easyif.h
instead 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_CURLDEBUG
in 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.TESTING
toTESTBUILD
andTESTRUN
.TESTBUILD
defaulting toTESTRUN
.