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

[vszakats]

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: 1a62b6e68c08c7e471ff22dd92932aba7e026817 (2015-03-03) Ref: #13583 Closes #13592

---

• [x] Depends on #13643.
• [x] Rebase on #13654.
• [x] Rebase on #13668.
• [x] settle on ENABLE_DEBUG to always set DEBUGBUILD
• [x] 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]

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]

• 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. → #13654 (mitigation)
• New compiler warnings appeared when building the Release + ENABLE_DEBUG=ON combo. → #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]

I'm not quite sure that enabling -DDEBUGBUILD unconditionally on ENABLE_DEBUG=ON is the right way to go. Reason is that it makes it a bit too easy to enable debug features, which are fatal for security. This can affect existing build configs which happen to set ENABLE_DEBUG=ON while doing a Release build. With the patch, this would produce a debug build proper, while without it it was only enabling memory debugging (aka ENABLE_CURLDEBUG=ON), though probably that's also not desired for release builds.

Not doing anything leaves the ENABLE_DEBUG=ON + BUILD_TESTING=ON builds broken by default, which is also undesired (tests are very well runnable without debug info), though it likely affects few users outside our own CI infrastructure.

One way to solve this cleanly, is introducing a new option that has the new behaviour. This could be called ENABLE_DEBUGBUILD=ON. This could also not set -DCURLDEBUG to give more direct control over these options.

[vszakats]

Another option is to set -DDEBUGBUILD unconditionally when both ENABLE_DEBUG=ON and BUILD_TESTING=ON are set.

edit: Not good because BUILD_TESTING is ON by default (except when Perl is missing or CURL_DISABLE_TESTS is set.)

[vszakats]

Another solution is to require Debug builds for BUILD_TESTING. The disadvantage of this is that some might use CMAKE_C_FLAGS=-DDEBUGBUILD, which also works, but fails this condition.

[jay]

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.

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?

[vszakats]

What I've found so far is that running or building tests don't require DEBUGBUILD, but building them with CURLDEBUG does. The problem is that ENABLE_DEBUG always sets the latter, but sets the former only when building with cmake's Debug config type. I don't know why that choice was made (1a62b6e68c08c7e471ff22dd92932aba7e026817). Perhaps because in autotools the equivalent option --enable-debug also enables compiler debug options, like Debug config does? FWIW in autotools --enable-debug always sets DEBUGBUILD.

Changing ENABLE_DEBUG to also always set DEBUGBUILD would be the easiest fix. I'm also okay with making this option easier to use (that was actually my goal after trying setup tests with CMake in new CI jobs), but are we also okay that this changes build behavior just enough to accidentally enable DEBUGBUILD for the the ENABLE_DEBUG + default (no cmake config type) or Release/MinSizeRel/RelWithDebInfo config types? I we can agree that this is fine, I'm all for this solution.

Why DEBUGBUILD is required with CURLDEBUG to build certain tests? Based on CMake it seems that without CURLDDEBUG, unit tests are not built, so the problem doesn't surface. When bulding them, e.g. test1395 calls dedotdotify() which is declared with UNITTEST in lib/urlapi.c, where UNITTEST is set to static unless DEBUGBUILD is set.

So perhaps the root cause is that the UNITTEST macro should depend on CURLDEBUG and not DEBUGBUILD (or depend on both)? Ref 05b100aee247bb9bec8e9a1b0166496aa4248d1c.

edit: There seems to be somewhat of a mixup in using CURLDEBUG vs. DEBUGBUILD in the code (also probably in my understanding about which does what in practice). Most of the time both are defined, so mixing them up often didn't cause failures I guess. Also, conditions for building unit tests are different in autotools and cmake, adding to the confusion. In CMake they're built when ENABLE_CURLDEBUG or ENABLE_DEBUG is set. In autotools they're built when --enable-debug is set plus some extra conditions (navigating around odd build issues). CMake also has this comment "running unittests require curl to compiled with CURLDEBUG" (added in 2023), which isn't confirmed by autotools behaviour. It's quite a maze.

[vszakats]

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

[vszakats]

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]

@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]

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.

[jay]

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.