Add Assure() as a replacement for problematic Must() (#864)

The now-deprecated Must() has several interdependent flaws:

1. Must() logs at debug level 3, hiding some important bugs from humans.

2. Must() has been (ab)used for both checking code logic and validating
   input, making purpose-specific implementation changes impractical.

3. Must() does not honor the standard NDEBUG macro, complicating runtime
   cost assessment and surprising some developers that are used to that
   standard assert() semantics.

The new Assure() is a throwing version of a POSIX assert(3):

* Meant for detecting Squid code logic bugs (not input validation).
* Informs admins about bugs by logging their info to cache.log.
* Completely disabled in NDEBUG builds.
* Kills the current component (e.g., a Squid-origin HTTP transaction).

The killed component boundary is essentially defined by the location of
the handling try/catch statement. Some Assure() failures will kill a
Squid process, but the throwing code should not worry (or even know)
about the catcher location and handling logic.

This change also optimizes the compiled Must()/Assure() caller code
size, which may help a bit with runtime performance: With the new
Assure()/Must() implementation approach, the total stripped Squid
executable size in one reasonable configuration goes down by 5%. For
comparison, removing all Must()s completely gives 6% size reduction.

Automatically replacing Must() calls with Assure() is not practical due
to the second flaw itemized above.

diff --git a/src/base/Assure.cc b/src/base/Assure.cc
new file mode 100644 (file)
index 0000000..d9c5ef1
--- /dev/null
+++ b/src/base/Assure.cc
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 1996-2022 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#include "squid.h"
+#include "base/Assure.h"
+#include "base/TextException.h"
+#include "debug/Stream.h"
+
+[[ noreturn ]] void
+ReportAndThrow_(const int debugLevel, const char *description, const SourceLocation &location)
+{
+    const TextException ex(description, location);
+    const auto label = debugLevel <= DBG_IMPORTANT ? "ERROR: Squid BUG: " : "";
+    // TODO: Consider also printing the number of BUGs reported so far. It would
+    // require GC, but we could even print the number of same-location reports.
+    debugs(0, debugLevel, label << ex);
+    throw ex;
+}
+

diff --git a/src/base/Assure.h b/src/base/Assure.h
new file mode 100644 (file)
index 0000000..bb571d2
--- /dev/null
+++ b/src/base/Assure.h
@@ -0,0 +1,52 @@
+/*
+ * Copyright (C) 1996-2022 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#ifndef SQUID_SRC_BASE_ASSURE_H
+#define SQUID_SRC_BASE_ASSURE_H
+
+#include "base/Here.h"
+
+/// Reports the description (at the given debugging level) and throws
+/// the corresponding exception. Reduces compiled code size of Assure() and
+/// Must() callers. Do not call directly; use Assure() instead.
+/// \param description explains the condition (i.e. what MUST happen)
+[[ noreturn ]] void ReportAndThrow_(int debugLevel, const char *description, const SourceLocation &);
+
+/// Calls ReportAndThrow() if needed. Reduces caller code duplication.
+/// Do not call directly; use Assure() instead.
+/// \param description c-string explaining the condition (i.e. what MUST happen)
+#define Assure_(debugLevel, condition, description, location) \
+    while (!(condition)) \
+        ReportAndThrow_((debugLevel), (description), (location))
+
+#if !defined(NDEBUG)
+
+/// Like assert() but throws an exception instead of aborting the process. Use
+/// this macro to detect code logic mistakes (i.e. bugs) where aborting the
+/// current AsyncJob or a similar task is unlikely to jeopardize Squid service
+/// integrity. For example, this macro is _not_ appropriate for detecting bugs
+/// that indicate a dangerous global state corruption which may go unnoticed by
+/// other jobs after the current job or task is aborted.
+#define Assure(condition) \
+        Assure2((condition), #condition)
+
+/// Like Assure() but allows the caller to customize the exception message.
+/// \param description string literal describing the condition (i.e. what MUST happen)
+#define Assure2(condition, description) \
+        Assure_(0, (condition), ("assurance failed: " description), Here())
+
+#else
+
+/* do-nothing implementations for NDEBUG builds */
+#define Assure(condition) ((void)0)
+#define Assure2(condition, description) ((void)0)
+
+#endif /* NDEBUG */
+
+#endif /* SQUID_SRC_BASE_ASSURE_H */
+

diff --git a/src/base/Makefile.am b/src/base/Makefile.am
index a5d35bd26cc4fa9eca00468580d9d91458126e8b..90b70855c5a073f1520e0f0f1f83d71085f3410b 100644 (file)
--- a/src/base/Makefile.am
+++ b/src/base/Makefile.am
@@ -11,6 +11,8 @@ include $(top_srcdir)/src/TestHeaders.am
 noinst_LTLIBRARIES = libbase.la
 
 libbase_la_SOURCES = \
+       Assure.cc \
+       Assure.h \
        AsyncCall.cc \
        AsyncCall.h \
        AsyncCallQueue.cc \

diff --git a/src/base/TextException.h b/src/base/TextException.h
index 307923909b0a9b400dcb8232bc06d7f32bc198aa..c413a22ac606fe90f5164abebfbea6d6b2232e8f 100644 (file)
--- a/src/base/TextException.h
+++ b/src/base/TextException.h
@@ -9,6 +9,7 @@
 #ifndef SQUID__TEXTEXCEPTION_H
 #define SQUID__TEXTEXCEPTION_H
 
+#include "base/Assure.h"
 #include "base/Here.h"
 
 #include <stdexcept>
@@ -57,19 +58,16 @@ std::ostream &operator <<(std::ostream &, const TextException &);
 /// legacy convenience macro; it is not difficult to type Here() now
 #define TexcHere(msg) TextException((msg), Here())
 
-/// Like assert() but throws an exception instead of aborting the process
-/// and allows the caller to customize the exception message and location.
+/// Like Must() but supports custom exception message and location.
 /// \param description string literal describing the condition; what MUST happen
+/// Deprecated: Use Assure2() for code logic checks and throw explicitly when
+/// input validation fails.
 #define Must3(condition, description, location) \
-    do { \
-        if (!(condition)) { \
-            const TextException Must_ex_(("check failed: " description), (location)); \
-            debugs(0, 3, Must_ex_.what()); \
-            throw Must_ex_; \
-        } \
-    } while (/*CONSTCOND*/ false)
-
-/// Like assert() but throws an exception instead of aborting the process.
+    Assure_(3, (condition), ("check failed: " description), (location))
+
+/// Like Assure() but only logs the exception if level-3 debugging is enabled
+/// and runs even when NDEBUG macro is defined. Deprecated: Use Assure() for
+/// code logic checks and throw explicitly when input validation fails.
 #define Must(condition) Must3((condition), #condition, Here())
 
 /// Reports and swallows all exceptions to prevent compiler warnings and runtime