cmake: `ENABLE_DEBUG=ON` to always set `-DDEBUGBUILD` by vszakats · Pull Request #13592 · curl/curl

vszakats · 2024-05-11T02:04:02Z

Before this patch ENABLE_DEBUG=ON always enabled the TrackMemory
(aka ENABLE_CURLDEBUG=ON) feature, but required the Debug CMake
configration to actually enable curl debug features
(aka -DDEBUGBUILD).

Curl debug features do not require compiling with C debug options. This
also made enabling debug features unintuitive and complicated to use.
Due to other issues (subject to PR #13694) it also caused an error in
default (and Release/MinSizeRel/RelWithDebInfo) configs, when
building the testdeps target:

ld: CMakeFiles/unit1395.dir/unit1395.c.o: in function `test':
unit1395.c:(.text+0x1a0): undefined reference to `dedotdotify'

Ref: https://github.com/curl/curl/actions/runs/9037287098/job/24835990826#step:3:2483

Fix it by always defining DEBUGBUILD when setting ENABLE_DEBUG=ON.
Decoupling this option from the selected CMake configuration.

Note that after this patch ENABLE_DEBUG=ON unconditionally enables
curl debug features. These features are insecure and unsuited for
production. Make sure to omit this option when building for production
in default, Release (and other not-Debug) modes.

Also delete a workaround no longer necessary in GHA CI jobs.

Ref: 1a62b6e (2015-03-03)
Ref: #13583
Closes #13592

Depends on lib: fix compiler warnings (gcc) #13643.
Rebase on appveyor: guard against crash-build with VS2008 #13654.
Rebase on cmake: initialize BUILD_TESTING before first use #13668.
settle on ENABLE_DEBUG to always set DEBUGBUILD
try untangling DEBUGBUILD / CURLDEBUG / UNITTESTS guards. AFAIU the first two should work independently of each other, whether we are building/running tests or not, whether we are building with debug info or not. → in separate PR.

vszakats · 2024-05-11T08:31:51Z

MSVC build with ENABLE_DEBUG=ON and Release; curl --version crashes:
https://ci.appveyor.com/project/curlorg/curl/builds/49792835/job/eup3qgdx5jgt9wk9
vs working (without this PR):
https://ci.appveyor.com/project/curlorg/curl/builds/49792583/job/490v4iri6qwrs4i5

vszakats · 2024-05-12T11:15:40Z

A Debug variant of CMake, VS2008, x86, Schannel, Build-only segfaults on startup with and without this patch, in shared builds. (unrelated to Unity mode.) One necessary trigger for this is DEBUGBUILD defined. → appveyor: guard against crash-build with VS2008 #13654 (mitigation)
New compiler warnings appeared when building the Release + ENABLE_DEBUG=ON combo. → lib: fix compiler warnings (gcc) #13643
Could not yet figure out how to make the VS2008 job verbose. It'd help seeing the actual compiler command-lines. (--verbose works, but with VS2008 it still doesn't show the compiler command-lines.)

Seen when setting `-DDEBUGBUILD` for mingw-w64 gcc 13.2.0 CMake unity builds in 'Release' configurations. ``` curl/lib/curl_gethostname.c:71:5: error: 'strncpy' specified bound 1025 equals destination size [-Werror=stringop-truncation] 71 | strncpy(name, force_hostname, namelen); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from curl/_bld/lib/CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c:175: In function 'hostcache_timestamp_remove', inlined from 'Curl_hash_clean_with_criterium' at curl/lib/hash.c:265:19, inlined from 'Curl_hash_clean_with_criterium' at curl/lib/hash.c:247:1, inlined from 'hostcache_prune' at curl/lib/hostip.c:228:3, inlined from 'Curl_hostcache_prune' at curl/lib/hostip.c:256:21: curl/lib/hostip.c:205:12: error: 'now' may be used uninitialized [-Werror=maybe-uninitialized] 205 | time_t age = prune->now - c->timestamp; | ^~~ curl/lib/hostip.c: In function 'Curl_hostcache_prune': curl/lib/hostip.c:241:10: note: 'now' was declared here 241 | time_t now; | ^~~ In function 'hostcache_timestamp_remove', inlined from 'fetch_addr' at curl/lib/hostip.c:310:8: curl/lib/hostip.c:205:23: error: 'user.now' may be used uninitialized [-Werror=maybe-uninitialized] 205 | time_t age = prune->now - c->timestamp; | ~~~~~^~~~~ curl/lib/hostip.c: In function 'fetch_addr': curl/lib/hostip.c:304:33: note: 'user' declared here 304 | struct hostcache_prune_data user; | ^~~~ In file included from curl/_bld/lib/CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c:40: curl/lib/cf-socket.c: In function 'cf_socket_send': curl/lib/cf-socket.c:1294:10: error: 'c' may be used uninitialized [-Werror=maybe-uninitialized] 1294 | if(c >= ((100-ctx->wblock_percent)*256/100)) { | ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ curl/lib/cf-socket.c:1292:19: note: 'c' was declared here 1292 | unsigned char c; | ^ In file included from curl/_bld/lib/CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c:364: In function 'tftp_state_timeout', inlined from 'tftp_multi_statemach' at curl/lib/tftp.c:1230:27: curl/lib/tftp.c:1208:5: error: 'current' may be used uninitialized [-Werror=maybe-uninitialized] 1208 | if(current > state->rx_time + state->retry_time) { | ^ curl/lib/tftp.c: In function 'tftp_multi_statemach': curl/lib/tftp.c:1192:10: note: 'current' was declared here 1192 | time_t current; | ^~~~~~~ ``` Ref: https://ci.appveyor.com/project/curlorg/curl/builds/49792835/job/91c8dj5qb36spfe0#L112 Ref: https://github.com/curl/curl/actions/runs/9082968838/job/24960616145#step:12:62 Ref: curl#13592 Closes curl#13643

The combination of `-DDEBUGBUILD`, a shared `curl.exe`, and the VS2008 compiler creates a `curl.exe` segfaulting on startup: ``` + _bld/src/curl.exe --version ./appveyor.sh: line 122: 793 Segmentation fault "${curl}" --version Command exited with code 139 ``` Ref: https://ci.appveyor.com/project/curlorg/curl/builds/49817266/job/651iy6qn1e238pqj#L191 Add job that triggers the issue and add the necessary logic to skip running the affected `curl.exe`. Ref: #13592 Closes #13654

Seen when setting `ENABLE_DEBUG=ON` and `-DDEBUGBUILD` for mingw-w64 gcc 13.2.0 CMake unity builds in 'Release' configurations. ``` curl/lib/curl_gethostname.c:71:5: error: 'strncpy' specified bound 1025 equals destination size [-Werror=stringop-truncation] 71 | strncpy(name, force_hostname, namelen); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from curl/_bld/lib/CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c:175: In function 'hostcache_timestamp_remove', inlined from 'Curl_hash_clean_with_criterium' at curl/lib/hash.c:265:19, inlined from 'Curl_hash_clean_with_criterium' at curl/lib/hash.c:247:1, inlined from 'hostcache_prune' at curl/lib/hostip.c:228:3, inlined from 'Curl_hostcache_prune' at curl/lib/hostip.c:256:21: curl/lib/hostip.c:205:12: error: 'now' may be used uninitialized [-Werror=maybe-uninitialized] 205 | time_t age = prune->now - c->timestamp; | ^~~ curl/lib/hostip.c: In function 'Curl_hostcache_prune': curl/lib/hostip.c:241:10: note: 'now' was declared here 241 | time_t now; | ^~~ In function 'hostcache_timestamp_remove', inlined from 'fetch_addr' at curl/lib/hostip.c:310:8: curl/lib/hostip.c:205:23: error: 'user.now' may be used uninitialized [-Werror=maybe-uninitialized] 205 | time_t age = prune->now - c->timestamp; | ~~~~~^~~~~ curl/lib/hostip.c: In function 'fetch_addr': curl/lib/hostip.c:304:33: note: 'user' declared here 304 | struct hostcache_prune_data user; | ^~~~ In file included from curl/_bld/lib/CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c:40: curl/lib/cf-socket.c: In function 'cf_socket_send': curl/lib/cf-socket.c:1294:10: error: 'c' may be used uninitialized [-Werror=maybe-uninitialized] 1294 | if(c >= ((100-ctx->wblock_percent)*256/100)) { | ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ curl/lib/cf-socket.c:1292:19: note: 'c' was declared here 1292 | unsigned char c; | ^ In file included from curl/_bld/lib/CMakeFiles/libcurl_object.dir/Unity/unity_0_c.c:364: In function 'tftp_state_timeout', inlined from 'tftp_multi_statemach' at curl/lib/tftp.c:1230:27: curl/lib/tftp.c:1208:5: error: 'current' may be used uninitialized [-Werror=maybe-uninitialized] 1208 | if(current > state->rx_time + state->retry_time) { | ^ curl/lib/tftp.c: In function 'tftp_multi_statemach': curl/lib/tftp.c:1192:10: note: 'current' was declared here 1192 | time_t current; | ^~~~~~~ ``` Ref: https://ci.appveyor.com/project/curlorg/curl/builds/49792835/job/91c8dj5qb36spfe0#L112 Ref: https://github.com/curl/curl/actions/runs/9082968838/job/24960616145#step:12:62 Ref: #13592 Closes #13643

vszakats · 2024-05-17T10:27:08Z

Settled with the original concept to always enable -DDEBUGBUILD when ENABLE_DEBUG=ON.

vszakats · 2024-05-20T00:12:18Z

I take issue with "too easy" being a bad thing. My thinking is if the user has made a choice to enable curl debug builds then why would we not honor that. Also, that's the way we already do it for autotools, ~~winbuild~~ (edit: winbuild has a DEBUG=yes but only to enable compiler debug flags) and cmake. So my opinion is leave that option as is.

@jay: This patch now makes ENABLE_DEBUG=ON honor this request and enables curl debug feature regardless of compiler debug flags. Before this patch it required a cmake Debug build. Do you agree with the change?

As far as another option, I have to wonder why does running the test suite in cmake require DEBUGBUILD? Couldn't that be the problem instead of adding DEBUGBUILD with another option?

I chased that down, which became a tree of fixes, with its main portion in #13694.

(There is no new option to enable DEBUGBUILD, ENABLE_DEBUG=ON does that now unconditionally.)

jay · 2024-05-20T07:51:21Z

@jay: This patch now makes ENABLE_DEBUG=ON honor this request and enables curl debug feature regardless of compiler debug flags. Before this patch it required a cmake Debug build. Do you agree with the change?

Seems right to me since it's documented as enabling curl debug features. It's confusing that --enable-debug and ENABLE_DEBUG are not exactly the same since ENABLE_DEBUG does not actually enable compiler debugging. But then again that's not the way cmake works. I'm not sure what we can or should do about that.

vszakats · 2024-05-20T08:30:27Z

Seems right to me since it's documented as enabling curl debug features.

Thanks!

It's confusing that --enable-debug and ENABLE_DEBUG are not exactly the same since ENABLE_DEBUG does not actually enable compiler debugging. But then again that's not the way cmake works. I'm not sure what we can or should do about that.

Agreed.

What we could do, is setting ENABLE_DEBUG to ON by default when the Debug configuration is selected. That seems like a natural fit for CMake and also matches what autotools does, assuming --enable-debug is the counterpart of --config/CMAKE_BUILD_TYPE = Debug in CMake.

When `CMAKE_BUILD_TYPE` is set to `Debug`, enable curl debug features by default. We do this to more closely match how autotools builds work, where `--enable-debug` enables both compiler debug feature and curl ones. You can override this by explicitly setting `ENABLE_DEBUG=OFF` and disable curl debug feature while leaving compiler debug enabled. Ref: curl#13592 (comment) Closes #xxxxx

Ref: curl/curl#13592

jay · 2024-05-21T06:56:50Z

What we could do, is setting ENABLE_DEBUG to ON by default when the Debug configuration is selected. That seems like a natural fit for CMake and also matches what autotools does, assuming --enable-debug is the counterpart of --config/CMAKE_BUILD_TYPE = Debug in CMake.

cmake is just different from autotools and the user expectations are different. For example a user may specify a debug configuration because they want compiler debug settings but not curl's DEBUGBUILD. I think it is better to leave it as a separate option, even if that's not what autotools is doing, and not enable it by default for debug configurations.

`ENABLE_DEBUG=ON` enables curl-level debug features. Building and running the curl test suite requires setting this option. Before this patch, while building the `testdeps` target with debug features on with the default ('Release') CMake config, the build stopped with this linker error: ``` ld: CMakeFiles/unit1395.dir/unit1395.c.o: in function `test': unit1395.c:(.text+0x1a0): undefined reference to `dedotdotify' A failure has been detected in another branch of the parallel make ``` Ref: https://github.com/curl/curl/actions/runs/9037287098/job/24835990826#step:3:2483 It happened because `dedotdotify` compiled as a static function into `libcurlu` due to the undefined `DEBUGBUILD` macro. Then `unit1395` failed to link it. Even though the build requires the `DEBUGBUILD` macro, our CMake logic defined it when building with CMake's 'Debug' configuration only. That configuration is not required to build or run the test suite, nor to use curl-level debug features. This patch fixes this by always defining `DEBUGBUILD` when setting `ENABLE_DEBUG=ON`. Decoupling this custom option from the selected CMake Release/Debug configuration. This change may also allow dropping Debug mode in AppVeyor CMake builds, possibly making them build faster and run smoother. Ref: curl#13583 Closes curl#13592

- fix `DEBUGBUILD` guards that should be `UNITTESTS`, in libcurl code used by unit tests. - fix guards for libcurl functions used in unit tests only. - sync `UNITTEST` attribute between declarations and definitions. - drop `DEBUGBUILD` guard from test `unit2600`. - fix guards for libcurl HSTS code used by both a unit test (`unit1660`) and `test0446`. - update an existing AppVeyor CI job to test the issues fixed. This fixes building tests with `CURLDEBUG` enabled but `DEBUGBUILD` disabled. This can happen when building tests with CMake with `ENABLE_DEBUG=ON` in Release config, or with `ENABLE_CURLDEBUG=ON` and _without_ `ENABLE_DEBUG=ON`. Possibly also with autotools when using `--enable-curldebug` without `--enable-debug`. Test results: - before: https://ci.appveyor.com/project/curlorg/curl/builds/49835609 https://ci.appveyor.com/project/curlorg/curl/builds/49898529/job/k8qpbs8idby70smw https://github.com/curl/curl/actions/runs/9259078835/job/25470318167?pr=13798#step:13:821 - after: https://ci.appveyor.com/project/curlorg/curl/builds/49839255 (the two failures are unrelated, subject to PR #13705) Ref: #13592 (issue discovery) Ref: #13689 (CI testing this PR with `DEBUGBUILD`/`CURLDEBUG` combinations) Closes #13694

vszakats added cmake tests labels May 11, 2024

github-actions bot added build cmdline tool labels May 11, 2024

vszakats marked this pull request as draft May 11, 2024 08:29

This was referenced May 11, 2024

GHA: add NetBSD, OpenBSD, FreeBSD/arm64 and OmniOS jobs #13583

Closed

[TESTING] appveyor: test running tests with Release builds #13593

Closed

github-actions bot added the CI Continuous Integration label May 11, 2024

vszakats force-pushed the cmake-debug-fixup branch 4 times, most recently from aedce29 to a192f59 Compare May 12, 2024 10:23

vszakats force-pushed the cmake-debug-fixup branch from 016ffb9 to 2db63ea Compare May 13, 2024 12:06

vszakats force-pushed the cmake-debug-fixup branch from 7395696 to 7241404 Compare May 14, 2024 11:31

vszakats mentioned this pull request May 14, 2024

lib: fix compiler warnings (gcc) #13643

Closed

vszakats force-pushed the cmake-debug-fixup branch from e81b1cc to bef6979 Compare May 14, 2024 15:49

vszakats mentioned this pull request May 15, 2024

appveyor: guard against crash-build with VS2008 #13654

Closed

vszakats force-pushed the cmake-debug-fixup branch from bef6979 to 88f8b48 Compare May 15, 2024 12:28

vszakats force-pushed the cmake-debug-fixup branch from 88f8b48 to e595bdb Compare May 15, 2024 12:58

vszakats force-pushed the cmake-debug-fixup branch from 4432b04 to 8323f8b Compare May 17, 2024 10:08

vszakats changed the title ~~cmake: fix ENABLE_DEBUG=ON builds in default/Release configs~~ cmake: fix ENABLE_DEBUG=ON to always set -DDEBUGBUILD May 17, 2024

vszakats marked this pull request as ready for review May 17, 2024 10:36

vszakats changed the title ~~cmake: fix ENABLE_DEBUG=ON to always set -DDEBUGBUILD~~ cmake: ENABLE_DEBUG=ON to always set -DDEBUGBUILD May 17, 2024

vszakats removed cmdline tool build labels May 17, 2024

vszakats mentioned this pull request May 17, 2024

build: untangle UNITTESTS and DEBUGBUILD macros #13694

Closed

vszakats force-pushed the cmake-debug-fixup branch from dba2b9f to 85432e5 Compare May 18, 2024 19:49

vszakats mentioned this pull request May 20, 2024

cmake: set ENABLE_DEBUG=ON by default for Debug builds #13717

Closed

vszakats added a commit to curl/curl-for-win that referenced this pull request May 20, 2024

curl.sh: add future update pending a curl PR [ci skip]

515bd24

Ref: curl/curl#13592

vszakats force-pushed the cmake-debug-fixup branch from 85432e5 to 3512410 Compare May 20, 2024 23:29

vszakats mentioned this pull request May 21, 2024

build: untangle CURLDEBUG and DEBUGBUILD macros #13718

Closed

6 tasks

vszakats added 3 commits May 27, 2024 14:07

GHA: test running tests with Release builds

1d1642c

GHA: delete DEBUGBUILD workaround no longer necessary

7d25adc

vszakats force-pushed the cmake-debug-fixup branch from 3512410 to 7d25adc Compare May 27, 2024 12:10

vszakats closed this in ea98445 May 27, 2024

vszakats deleted the cmake-debug-fixup branch May 27, 2024 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

cmake: `ENABLE_DEBUG=ON` to always set `-DDEBUGBUILD`#13592

cmake: `ENABLE_DEBUG=ON` to always set `-DDEBUGBUILD`#13592
vszakats wants to merge 3 commits intocurl:masterfrom
vszakats:cmake-debug-fixup

vszakats commented May 11, 2024 •

edited

Loading

Uh oh!

vszakats commented May 11, 2024 •

edited

Loading

Uh oh!

vszakats commented May 12, 2024 •

edited

Loading

Uh oh!

vszakats commented May 17, 2024

Uh oh!

vszakats commented May 20, 2024

Uh oh!

jay commented May 20, 2024

Uh oh!

vszakats commented May 20, 2024 •

edited

Loading

Uh oh!

jay commented May 21, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

vszakats commented May 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vszakats commented May 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vszakats commented May 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vszakats commented May 17, 2024

Uh oh!

vszakats commented May 20, 2024

Uh oh!

jay commented May 20, 2024

Uh oh!

vszakats commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jay commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

vszakats commented May 11, 2024 •

edited

Loading

vszakats commented May 11, 2024 •

edited

Loading

vszakats commented May 12, 2024 •

edited

Loading

vszakats commented May 20, 2024 •

edited

Loading

jay commented May 21, 2024 •

edited

Loading