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: ENABLE_DEBUG=ON to always set -DDEBUGBUILD #13592

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented May 11, 2024

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


@vszakats
Copy link
Member Author

vszakats commented May 11, 2024

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

@github-actions github-actions bot added the CI Continuous Integration label May 11, 2024
@vszakats vszakats force-pushed the cmake-debug-fixup branch 4 times, most recently from aedce29 to a192f59 Compare May 12, 2024 10:23
vszakats added a commit to vszakats/curl that referenced this pull request May 12, 2024
`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
@vszakats
Copy link
Member Author

vszakats commented May 12, 2024

  • 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.)

vszakats added a commit to vszakats/curl that referenced this pull request May 13, 2024
`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
vszakats added a commit to vszakats/curl that referenced this pull request May 14, 2024
`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
vszakats added a commit to vszakats/curl that referenced this pull request May 14, 2024
Seen when setting `-DDEBUGBUILD` for mingw-w64 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: curl#13592
Closes #xxxxx
vszakats added a commit to vszakats/curl that referenced this pull request May 14, 2024
`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
vszakats added a commit to vszakats/curl that referenced this pull request May 14, 2024
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
vszakats added a commit that referenced this pull request May 15, 2024
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
vszakats added a commit to vszakats/curl that referenced this pull request May 15, 2024
`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
vszakats added a commit to vszakats/curl that referenced this pull request May 15, 2024
`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
vszakats added a commit to vszakats/curl that referenced this pull request May 16, 2024
`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
vszakats added a commit that referenced this pull request May 17, 2024
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 added a commit to vszakats/curl that referenced this pull request May 17, 2024
`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
@vszakats 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
Copy link
Member Author

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

@vszakats vszakats marked this pull request as ready for review May 17, 2024 10:36
@vszakats 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 added a commit to vszakats/curl that referenced this pull request May 18, 2024
`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
@vszakats
Copy link
Member Author

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
Copy link
Member

jay commented May 20, 2024

@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
Copy link
Member Author

vszakats commented May 20, 2024

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.

vszakats added a commit to vszakats/curl that referenced this pull request May 20, 2024
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
vszakats added a commit to curl/curl-for-win that referenced this pull request May 20, 2024
`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
@jay
Copy link
Member

jay commented May 21, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration cmake tests
Development

Successfully merging this pull request may close these issues.

None yet

2 participants