curl_setup: asking for DEBUGBUILD undefs NDEBUG

[bagder]

A debug build enables DEBUGASSERT(), but the NDEBUG define actively disables asserts.

Ref: #22481

Discussion item.

[jay]

hm I think you could lose the "make a decision" part...

Suggested change

	#error "a debug build with NDEBUG defined is a mixed message. Make a decision."
	#error "NDEBUG cannot be defined with DEBUGBUILD"

I haven't read #22481 yet but NDEBUG is used to disable asserts, like in Visual Studio "Release" configurations. And no I don't know why anyone would want to do that in a debug build, but if there is a valid use case I don't see anything wrong with allowing it. (edit: changed my mind after reading #22481, specifically that NDEBUGs may be inadvertent, added in a way that can't easily be controlled, and the intention of DEBUGASSERTs to work for debug builds)

[bagder]

It complicates our lives if code in DEBUGASSERT() have a different condition for existing than code within DEBUGBUILD, as #22481 shows. For example, if we define a function within DEBUGBUILD conditions that is used only from within a DEBUGASSERT().

It seems like flexibility that isn't necessary, that complicates the code.

[bagder]

I think you could lose the "make a decision" part

Agreed!

[vszakats]

It think may be useful to lock debug-enabled builds to require !NDEBUG,
because it makes it less likely to be used in production builds, and since they
are inherently unsafe, this may prevent some (possibly accidental) mis-uses.

But, thinking about undesired side-effects in particular in CMake builds, this
makes it non-trivial to run tests (or mem trace) with compiler optimizations
enabled, because CMake sets NDEBUG in all configurations, except Debug.
I was thinking it may not set it for RelWithDebInfo, but it does.

It also makes it more difficult in general to freely toggle, traditionally
independent, settings (needing extra logic, e.g. in build-fuzzers). Further,
it will inevitably cause DEBUGBUILD- and NDEBUG-dependent internals to
tangle up in the codebase. Untangling these may or may not be fun, but
also easily prevented now.

Is this worth fixing an IMO, super rare, easily avoidable, and just easily
fixable issue as in the original report? It feels like overcorrecting to me.

So far I'm left unconvinced that this patch is overall causing more good
than drawbacks.

[icing]

It also makes it more difficult in general to freely toggle, traditionally independent, settings (needing extra logic, e.g. in build-fuzzers). Further, it will inevitably cause DEBUGBUILD- and NDEBUG-dependent internals to tangle up in the codebase. Untangling these may or may not be fun, but also easily prevented now.

I do not understand what you mean. Could you give a concrete example, so we can understand the impact that you are seeing? We fail to do that right now and re-stating generalities does not help.

[bagder]

it will inevitably cause DEBUGBUILD- and NDEBUG-dependent internals to tangle up in the codebase

They are tangled up now in a way that is unexpected and surprising to me! And the CI build proves that (some) users also probably do those builds without meaning it.

[icing]

To make the "super rare, easily avoidable" situation more clear. My problem is that the following pattern does not compile with DEBUGBUILD and NDEBUG right now:

#ifdef DEBUGBUILD
static bool fn(params) { ... }
#endif 
...
  DEBUGASSERT(fn(y, z, z));

fails with error "unused function fn". Now, the "easy" solution suggested by @vszakats is to change this to

#if defined(DEBUGBUILD) && !defined(NDEBUG) 
static bool fn(params) { ... }
#endif 
...
  DEBUGASSERT(fn(y, z, z));

but this will fail with "unknown function fn" when an application CURL_DEBUGASSERT and NDEBUG is defined.

So, we have a complication in our build variability and I am merely asking how I should write code to avoid this mess. And it is not "super rare, easily avoidable" from my point of view.

Now, I try to be Mister Positive here and have tried the following change:

diff --git a/lib/curl_setup.h b/lib/curl_setup.h
index 9d18875b9d..4246a5cb4b 100644
--- a/lib/curl_setup.h
+++ b/lib/curl_setup.h
@@ -1075,6 +1075,10 @@ typedef unsigned int curl_bit;
 #ifdef CURL_DEBUGASSERT
 /* External assertion handler for custom integrations */
 #define DEBUGASSERT(x) CURL_DEBUGASSERT(x)
+#elif defined(NDEBUG)
+/* assert() is disabled, but we want the code to be referenced or
+ * we'll get compiler errors. */
+#define DEBUGASSERT(x) (void)(x)
 #else
 #define DEBUGASSERT(x) assert(x)
 #endif

which makes CI work for me. I still do not really like this, but it at least solves the compile failures.

[vszakats]

--- a/lib/curl_setup.h
+++ b/lib/curl_setup.h
@@ -1075,6 +1075,10 @@ typedef unsigned int curl_bit;
 #ifdef CURL_DEBUGASSERT
 /* External assertion handler for custom integrations */
 #define DEBUGASSERT(x) CURL_DEBUGASSERT(x)
+#elif defined(NDEBUG)
+/* assert() is disabled, but we want the code to be referenced or
+ * we'll get compiler errors. */
+#define DEBUGASSERT(x) (void)(x)
 #else
 #define DEBUGASSERT(x) assert(x)
 #endif

which makes CI work for me. I still do not really like this, but it at least solves the compile failures.

It works for me if we expect or want to encourage the pattern more.
Personally, I'd probably keep the assert() expression self-contained.
In this case the debug function is doing a loop and throws failf(),
which surprises me within an assert. I wonder if guarding the calls
for DEBUGBUILD, and unconditionally asserting on fail could work
as an alternative?:

--- a/lib/multi.c
+++ b/lib/multi.c
@@ -3682,7 +3682,10 @@ static void multi_deltimeout(struct Curl_easy *data, expire_id eid)
     }
     anchor = &timeouts->next[*anchor];
   }
-  DEBUGASSERT(multi_timeouts_check(data));
+#ifdef DEBUGBUILD
+  if(!multi_timeouts_check(data));
+    DEBUGASSERT(0);
+#endif
   if(timeouts->registered) {
     struct Curl_multi *multi = data->multi;
     int rc;
@@ -3738,7 +3741,10 @@ static CURLMcode multi_addtimeout(struct Curl_easy *data,
   timeouts->next[eid] = *anchor;
   timeouts->next[eid] = *anchor;
   *anchor = eid;
-  DEBUGASSERT(multi_timeouts_check(data));
+#ifdef DEBUGBUILD
+  if(!multi_timeouts_check(data));
+    DEBUGASSERT(0);
+#endif
   CURL_TRC_TIMER(data, eid, "set for %" FMT_TIMEDIFF_T "us",
                  curlx_ptimediff_us(stamp, Curl_pgrs_now(data)));
   return CURLM_OK;

Needs a few more lines, but unless this happens a lot in the code,
this is probably fine.

[vszakats]

Oh no :(

[bagder]

@vszakats you seem to be left alone in defending NDEBUG's "independence", which makes me now argue for this simple approach.

[vszakats]

Of any options, this is the one I prefer the least, by a (very) large margin.

It seems simple, yet it's the sneakiest method, by silently overriding
higher-level tools, settings, user intent and expectations, then making
it difficult to figure out what is happening, why and where. Ask me how I know.

This also neuters the only upside of the planned solutions, that is to prevent
someone creating an insecure debug-enabled build in non-Debug, aka Release
mode. This method will just silently make it work, while also adding __FILE__
info to make the insecure build leak local paths too.

This makes the problem worse.

Only to allow DEBUGASSERT(function())? It makes me confused how we'd
arrived here.

I don't see where the discussions happened, I haven't seen them.
The last message was yours yesterday saying you preferred the other
option, to which I agreed. Then I had question to think about, but the
answer to those is certainly not this solution IMO.

edit: if we absolutely want to prevent combining debug-enabled with
non-debug, I believe it should be a build error.

[icing]

Only to allow DEBUGASSERT(function())? It makes me confused how we'd arrived here.

Please do not oversimplify the other side's position. This is not helpful.

The function example is just one case where we stumbled upon the problem. Both @bagder and myself would have bet our life savings that DEBUGASSERT's get evaluated in a DEBUGBUILD. Such wrong assumptions are very nasty when writing code and analysing problems. You can spent days wondering on a customer report, who you told to enable debug, why that bloody assertion did not trigger.

Ask me how I know.

Same here.

[bagder]

if we absolutely want to prevent combining debug-enabled with non-debug, I believe it should be a build error.

I think we absolutely do, because I think this practice risks triggering or hiding a bigger problem at some point. Me and Stefan write a lot of curl code and we were both taken by surprise by this. I think that's a strong signal.

The reason I advocate just undef'ing NDEBUG is because exactly what was shown in the CI builds: several cmake magic combos make it automatically and seemingly without being asked, add -DNDEBUG to the compiler options. If we do not automatically undef this, we get an error instead and then we need to start debugging the cmake logic to figure out why this happens and tell it not to. It feels likely that such a magic NDEBUG addition to the command line can also happen to other users when they build curl and I don't think we need to bring them this problem. Because I see no particular reason to disable asserts in debug builds.

[vszakats]

Continuing the above, maybe you mean why the CMake Debug option and
ENABLE_DEBUG in addition?

I'm reading through the history.

Initially a debug-enabled build triggered when selecting both ENABLE_DEBUG
and building in Debug mode:

curl/CMakeLists.txt

Lines 83 to 91 in 1a62b6e

	if (ENABLE_DEBUG)
	# DEBUGBUILD will be defined only for Debug builds
	if(NOT CMAKE_VERSION VERSION_LESS 3.0)
	set_property(DIRECTORY APPEND PROPERTY COMPILE_DEFINITIONS $<$<CONFIG:Debug>:DEBUGBUILD>)
	else()
	set_property(DIRECTORY APPEND PROPERTY COMPILE_DEFINITIONS_DEBUG DEBUGBUILD)
	endif()
	set(ENABLE_CURLDEBUG ON)
	endif()

This sounds good, and would result in the two debug modes interlocked (similar
to what we may like now). It also meant that an ENABLE_DEBUG=ON build didn't
necessarily create a debug-enabled build, but silently so. Combined with the
fact that CMake's default build mode is largely undefined, this required to explicitly
set both this and the build mode to get a debug-enabled build. This still sounds
good, but in practice it made thing unpredicable, and almost impossible to
discover. It also caused broken builds, because the interlocking was only done
for DEBUGBUILD but not for CURLDEBUG, and these two were mixed up in the
code and basically one not working without the other in unpredicable ways. It
means there were 3 settings to discover and configure in careful harmony to get
debug-enabled successfully. This made me change it in 2024, to remove the
interlock (my fault after all!) in ea98445,
realizing these are 2-3 independent settings, in an attempt to improve things.
This was also critical to freely test build combinations, to untangle the internals,
leading to fixes fc8e0de and 9866e2e.
This was followed by changes aiming to isolate MemTrack and make it build cleanly
in all combinations. This then led to the mentioned option merge early this year.

The above puts the previously researched CI history in different light, because
all those quoted CI configurations did not actually do what they were set out
to do, e.g. an ENABLE_DEBUG=ON + Release build just silently did not build a
debug-enabled build, but a frankenbuild with CURLDEBUG only, or something
alike.

So, could just re-enabling the original interlock work now? I guess it could no
longer produce broken builds, because CURLDEBUG is no more, there is only
DEBUGBUILD. I'd expect locking it to Debug configs would build reliably. But,
it would still suffer from the opacity of the original solution: There is no way to
know what's being built at the end. There is no error, warning or indication of
the outcome. This creates suprises and makes configuration annoying and
error-prone, even if we document it.

This leads to the next issue with the interlock solution. CMake build mode is
not known at configure time (due to multi-config builds, MSVC/Xcode/Ninja
multiconfig), which means if the DEBUGBUILD setting depends on it, it's also
not known at configure time. This breaks current and future logic that depends
on knowing in advance if we build debug-enabled or not, a trivial one is the
Features: display (which was not implemented yet at the time of the original
commit, correction: it was, but without the Debug feature.).

In summary: I'd personally go with blocking the invalid combination at the
earliest stage (and the next stages), as much as possible, with relevant info
on what's missing and how to continue. This makes configurations well-defined,
transparent, predictable, and we can continue using all variables at configure
time. I also don't think a single, "one-click", trivially misunderstood option is
a good solution to enable basically dangerous development builds (as
autotools does now with --enable-debug, but perhaps those who use
autotools know better what they're doing.)

[bagder]

I also don't think a single, "one-click", trivially misunderstood option is
a good solution to enable basically dangerous development builds (as
autotools does now with --enable-debug

I don't understand. You enable debug-builds in cmake and configure using an option, almost the same way. How is this bad? and what would be better?

[vszakats]

I don't understand. You enable debug-builds in cmake and configure using an option, almost the same way. How is this bad? and what would be better?

Almost, but in CMake, ENABLE_DEBUG comes with a warning, and it also offers a distinct, standard (across all CMake projects) option to do "just Debug" (as in debug info, -g, aka Debug mode) builds.

I understand that's what we are set out to change, so that ENABLE_DEBUG requires also Debug mode (for the asserts), which means it needs another matching, built-in, setting, reducing the one-click issue a notch more.

---

That's probably my last post about this, I think I've researched all I could, and said all I know about this. It feels fairly extensive for what is, and at this point it'd probably be simpler to speak with that customer affected by this, assuming they exist. FWIW making sure a remote customer is using the build we meant them to be using is probably a separate topic altogether, starting from identifying all patches/options applied, confirming the build has them, and that it's the one actually run. Valid questions, happen almost every time when debugging locally too, and I think, a distinctive, unconditional trace message at the tested code path is usually a better solution for this. Certainly beats this storm IMO!

[bagder]

The "customers" who primarily were affected and triggered this change were me and @icing. We could not understand what was happening as we saw no reason to the logic. We know a little more now I think.

[bagder]

And really, there is no "storm". We just debate the best way to do it and we have different opinions. Having different views and ideas is good.

[bagder]

#22513 replaces this