Use a new macro trick to avoid throwing in destructors.

src/Debug.h

@@ -48,9 +48,11 @@ bool debug_is_active_impl(int verbosity, const char *file, const char *function,
  * is determined by the value of the environment variable
  * HL_DEBUG_CODEGEN
  */
+// clang-format off
 #define debug(n)                                     \
     /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \
-    (!debug_is_active((n))) ? (void)0 : ::Halide::Internal::Voidifier() & std::cerr
+    if (debug_is_active((n))) std::cerr
+// clang-format on
 
 /** Allow easily printing the contents of containers, or std::vector-like containers,
  *  in debug output. Used like so:

src/Error.cpp

@@ -150,13 +150,6 @@ template<typename T>
     }
 
 #ifdef HALIDE_WITH_EXCEPTIONS
-    if (std::uncaught_exceptions() > 0) {
-        // This should never happen - evaluating one of the arguments to the
-        // error message would have to throw an exception. Nonetheless, in
-        // case it does, preserve the exception already in flight and suppress
-        // this one.
-        std::rethrow_exception(std::current_exception());
-    }
     throw e;
 #else
     std::cerr << e.what() << std::flush;

src/Error.h

@@ -123,113 +123,95 @@ void issue_warning(const char *warning);
 
 template<typename T>
 struct ReportBase {
-    std::ostringstream msg;
-
-    ReportBase(const char *file, const char *function, int line, const char *condition_string, const char *prefix) {
-        if (debug_is_active_impl(1, file, function, line)) {
-            msg << prefix << " at " << file << ":" << line << ' ';
-            if (condition_string) {
-                msg << "Condition failed: " << condition_string << ' ';
-            }
-        }
-    }
-
-    // Just a trick used to convert RValue into LValue
-    HALIDE_ALWAYS_INLINE T &ref() {
-        return *static_cast<T *>(this);
-    }
-
     template<typename S>
     HALIDE_ALWAYS_INLINE T &operator<<(const S &x) {
         msg << x;
         return *static_cast<T *>(this);
     }
 
+    HALIDE_ALWAYS_INLINE operator bool() const {
+        return !finalized;
+    }
+
 protected:
+    std::ostringstream msg{};
+    bool finalized{false};
+
+    // This function is called as part of issue() below. We can't use a
+    // virtual function because issue() needs to be marked [[noreturn]]
+    // for errors and be left alone for warnings (i.e., they have
+    // different signatures).
     std::string finalize_message() {
         if (!msg.str().empty() && msg.str().back() != '\n') {
             msg << "\n";
         }
+        finalized = true;
         return msg.str();
     }
+
+    T &init(const char *file, const char *function, const int line, const char *condition_string, const char *prefix) {
+        if (debug_is_active_impl(1, file, function, line)) {
+            msg << prefix << " at " << file << ":" << line << ' ';
+            if (condition_string) {
+                msg << "Condition failed: " << condition_string << ' ';
+            }
+        }
+        return *static_cast<T *>(this);
+    }
 };
 
 template<typename Exception>
-struct ErrorReport : ReportBase<ErrorReport<Exception>> {
-    using Base = ReportBase<ErrorReport>;
-
-    ErrorReport(const char *file, const char *function, int line, const char *condition_string)
-        : Base(file, function, line, condition_string, Exception::error_name) {
-        this->msg << "Error: ";
+struct ErrorReport final : ReportBase<ErrorReport<Exception>> {
+    ErrorReport &init(const char *file, const char *function, const int line, const char *condition_string) {
+        return ReportBase<ErrorReport>::init(file, function, line, condition_string, Exception::error_name) << "Error: ";
     }
 
-#ifdef _MSC_VER
-#pragma warning(push)
-#pragma warning(disable : 4722)
-#endif
-    /** When you're done using << on the object, and let it fall out of
-     * scope, this throws an exception (or aborts if exceptions are disabled).
-     * This is a little dangerous because the destructor will also be
-     * called if there's an exception in flight due to an error in one
-     * of the arguments passed to operator<<. We handle this by rethrowing
-     * the current exception, if it exists.
-     */
-    [[noreturn]] ~ErrorReport() noexcept(false) {
+    [[noreturn]] void issue() noexcept(false) {
         throw_error(Exception(this->finalize_message()));
     }
-#ifdef _MSC_VER
-#pragma warning(pop)
-#endif
 };
 
-struct WarningReport : ReportBase<WarningReport> {
-    WarningReport(const char *file, const char *function, int line, const char *condition_string)
-        : ReportBase(file, function, line, condition_string, "Warning") {
-        this->msg << "Warning: ";
+struct WarningReport final : ReportBase<WarningReport> {
+    WarningReport &init(const char *file, const char *function, const int line, const char *condition_string) {
+        return ReportBase::init(file, function, line, condition_string, "Warning") << "Warning: ";
     }
 
-    /** When you're done using << on the object, and let it fall out of
-     * scope, this prints the computed warning message.
-     */
-    ~WarningReport() {
+    void issue() {
         issue_warning(this->finalize_message().c_str());
     }
 };
 
-// This uses operator precedence as a trick to avoid argument evaluation if
-// an assertion is true: it is intended to be used as part of the
-// _halide_internal_assertion macro, to coerce the result of the stream
-// expression to void (to match the condition-is-false case).
-struct Voidifier {
-    // This has to be an operator with a precedence lower than << but
-    // higher than ?:
-    template<typename T>
-    HALIDE_ALWAYS_INLINE void operator&(T &) {
-    }
-};
-
 /**
- * _halide_internal_assertion is used to implement our assertion macros
+ * _halide_internal_diagnostic is used to implement our assertion macros
  * in such a way that the messages output for the assertion are only
  * evaluated if the assertion's value is false.
  *
- * Note that this macro intentionally has no parens internally; in actual
- * use, the implicit grouping will end up being
- *
- *   condition ? (void) : (Voidifier() & (ErrorReport << arg1 << arg2 ... << argN))
- *
  * This (regrettably) requires a macro to work, but has the highly desirable
  * effect that all assertion parameters are totally skipped (not ever evaluated)
  * when the assertion is true.
+ *
+ * The macro works by deferring the call to issue() until after the stream
+ * has been evaluated. This previously used a trick where ErrorReport would
+ * throw in the destructor, but throwing in a destructor is UB in a lot of
+ * scenarios, and it was easy to break things by mistake.
  */
-#define _halide_internal_assertion(condition, type)  \
+// clang-format off
+#define _halide_internal_diagnostic(condition, type, condition_string)  \
     /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \
-    (condition) ? (void)0 : ::Halide::Internal::Voidifier() & ::Halide::Internal::ErrorReport<type>(__FILE__, __FUNCTION__, __LINE__, #condition).ref()
+    if (!(condition)) for (type _err; _err; _err.issue()) _err.init(__FILE__, __FUNCTION__, __LINE__, condition_string)
+// clang-format on
+
+#define _halide_internal_error(type) \
+    _halide_internal_diagnostic(0, Halide::Internal::ErrorReport<type>, nullptr)
+
+#define _halide_internal_assertion(condition, type) \
+    _halide_internal_diagnostic(condition, Halide::Internal::ErrorReport<type>, #condition)
+
+#define user_error _halide_internal_error(Halide::CompileError)
+#define internal_error _halide_internal_error(Halide::InternalError)
+#define halide_runtime_error _halide_internal_error(Halide::RuntimeError)
 
-#define internal_error Halide::Internal::ErrorReport<Halide::InternalError>(__FILE__, __FUNCTION__, __LINE__, nullptr)
-#define user_error Halide::Internal::ErrorReport<Halide::CompileError>(__FILE__, __FUNCTION__, __LINE__, nullptr)
-#define user_warning Halide::Internal::WarningReport(__FILE__, __FUNCTION__, __LINE__, nullptr)
-#define halide_runtime_error Halide::Internal::ErrorReport<Halide::RuntimeError>(__FILE__, __FUNCTION__, __LINE__, nullptr)
+#define user_warning _halide_internal_diagnostic(0, Halide::Internal::WarningReport, nullptr)
 
 #define internal_assert(c) _halide_internal_assertion(c, Halide::InternalError)
 #define user_assert(c) _halide_internal_assertion(c, Halide::CompileError)

src/autoschedulers/adams2019/AutoSchedule.cpp

@@ -58,7 +58,6 @@
 #include "Cache.h"
 #include "CostModel.h"
 #include "DefaultCostModel.h"
-#include "Errors.h"
 #include "Featurization.h"
 #include "FunctionDAG.h"
 #include "Halide.h"

src/autoschedulers/adams2019/FunctionDAG.h

@@ -13,9 +13,8 @@
 
 #include <vector>
 
-#include "Errors.h"
 #include "Featurization.h"
-#include "Halide.h"
+#include "HalidePlugin.h"
 
 namespace Halide {
 namespace Internal {

src/autoschedulers/anderson2021/AutoSchedule.cpp

@@ -48,7 +48,6 @@
 #include "AutoSchedule.h"
 #include "CostModel.h"
 #include "DefaultCostModel.h"
-#include "Errors.h"
 #include "Featurization.h"
 #include "FunctionDAG.h"
 #include "LoopNest.h"

src/autoschedulers/anderson2021/FunctionDAG.h

@@ -12,9 +12,8 @@
 #include <utility>
 #include <vector>
 
-#include "Errors.h"
 #include "Featurization.h"
-#include "Halide.h"
+#include "HalidePlugin.h"
 #include "PerfectHashMap.h"
 
 namespace Halide {

src/autoschedulers/anderson2021/GPULoopInfo.cpp

@@ -1,5 +1,4 @@
 #include "GPULoopInfo.h"
-#include "Errors.h"
 #include "LoopNest.h"
 
 namespace Halide {

src/autoschedulers/anderson2021/GPULoopInfo.h

@@ -10,7 +10,7 @@
 #include <memory>
 #include <vector>
 
-#include "Halide.h"
+#include "HalidePlugin.h"
 #include "ThreadInfo.h"
 
 namespace Halide {

src/autoschedulers/anderson2021/GPUMemInfo.h

@@ -6,7 +6,6 @@
 #include <vector>
 
 #include "ASLog.h"
-#include "Errors.h"
 
 /** \file
  *

src/autoschedulers/anderson2021/ThreadInfo.h

@@ -10,8 +10,8 @@
 
 #include <vector>
 
-#include "Errors.h"
 #include "FunctionDAG.h"
+#include "HalidePlugin.h"
 
 namespace Halide {
 namespace Internal {

src/autoschedulers/common/HalidePlugin.h

@@ -4,8 +4,6 @@
 #define HALIDE_KEEP_MACROS
 #include "Halide.h"
 
-#include "Errors.h"
-
 #define REGISTER_AUTOSCHEDULER(NAME)                                  \
     struct HALIDE_EXPORT Register##NAME {                             \
         Register##NAME() {                                            \

src/autoschedulers/common/ParamParser.h

@@ -1,7 +1,7 @@
 #ifndef PARSE_H
 #define PARSE_H
 
-#include "Errors.h"
+#include "HalidePlugin.h"
 #include <sstream>
 #include <type_traits>
 

src/autoschedulers/li2018/GradientAutoscheduler.cpp

@@ -1,6 +1,5 @@
 #include "HalidePlugin.h"
 
-#include "Errors.h"
 #include "ParamParser.h"
 
 namespace Halide {

test/correctness/custom_error_reporter.cpp

@@ -1,6 +1,6 @@
 #include "Halide.h"
-#include <stdio.h>
-#include <string.h>
+#include <cstdio>
+#include <cstdlib>
 
 using namespace Halide;
 
@@ -18,27 +18,22 @@ int should_never_be_evaluated() {
     return 0;
 }
 
-class MyCustomErrorReporter : public Halide::CompileTimeErrorReporter {
-public:
-    int errors_occurred;
-    int warnings_occurred;
-
-    MyCustomErrorReporter()
-        : errors_occurred(0), warnings_occurred(0) {
-    }
+struct MyCustomErrorReporter final : CompileTimeErrorReporter {
+    int errors_occurred{0};
+    int warnings_occurred{0};
 
     void warning(const char *msg) override {
-        auto msg_safe = Halide::Internal::replace_all(msg, ":", "(semicolon)");
+        const auto msg_safe = Internal::replace_all(msg, ":", "(colon)");
         printf("Custom warn: %s\n", msg_safe.c_str());
         warnings_occurred++;
     }
 
     [[noreturn]] void error(const char *msg) override {
         // Emitting "error.*:" to stdout or stderr will cause CMake to report the
-        // test as a failure on Windows, regardless of error code returned.
+        // test as a failure on Windows, regardless of the error code returned.
         // The error text we get from ErrorReport probably contains some variant
         // of this, so let's make sure it doesn't match that pattern.
-        auto msg_safe = Halide::Internal::replace_all(msg, ":", "(semicolon)");
+        const auto msg_safe = Internal::replace_all(msg, ":", "(colon)");
         printf("Custom err: %s\n", msg_safe.c_str());
         errors_occurred++;
 
@@ -56,16 +51,16 @@ class MyCustomErrorReporter : public Halide::CompileTimeErrorReporter {
 int main(int argc, char **argv) {
 
     // Use argc here so that the compiler cannot optimize it away:
-    // we know argc > 0 always, but compiler (probably) doesn't.
+    // we know argc > 0 always, but the compiler (probably) doesn't.
     _halide_user_assert(argc > 0) << should_never_be_evaluated();
 
     MyCustomErrorReporter reporter;
     set_custom_compile_time_error_reporter(&reporter);
 
-    Halide::Internal::WarningReport("", "", 0, nullptr) << "Here is a warning.";
+    user_warning << "Here is a warning.";
 
     // This call should not return.
-    _halide_user_assert(argc == 0) << should_be_evaluated();
+    user_assert(argc == 0) << should_be_evaluated();
 
     printf("CompileTimeErrorReporter::error() must not return.\n");
     return 1;