Skip to content

Commit

Permalink
Halide::Error should not extend std::runtime_error (#6927)
Browse files Browse the repository at this point in the history
* Halide::Error should not extend std::runtime_error

Unfortunately, the std error/exception classes aren't marked for DLLEXPORT under MSVC; we need our Error classes to be DLLEXPORT for libHalide (and python bindings). The current situation basically causes MSVC to generator another version of `std::runtime_error` marked for DLLEXPORT, which can lead to ODR violations, which are bad. AFAICT we don't really rely on this inheritance anywhere, so this just eliminates the inheritance entirely.

(Note that I can't point to a specific malfunction resulting from this, but casual googling based on the many warnings MSVC emits about the current situation has me convinced that it needs addressing.)

* noexcept
  • Loading branch information
steven-johnson committed Aug 10, 2022
1 parent 1bf1599 commit 92de4a1
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 13 deletions.
60 changes: 54 additions & 6 deletions src/Error.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,48 @@ bool exceptions_enabled() {
#endif
}

Error::Error(const char *msg)
: what_(new char[strlen(msg) + 1]) {
strcpy(what_, msg);
}

Error::Error(const std::string &msg)
: std::runtime_error(msg) {
: Error(msg.c_str()) {
}

Error::Error(const Error &that)
: Error(that.what_) {
}

Error &Error::operator=(const Error &that) {
if (this != &that) {
delete[] this->what_;
this->what_ = new char[strlen(that.what_) + 1];
strcpy(this->what_, that.what_);
}
return *this;
}

Error::Error(Error &&that) noexcept {
this->what_ = that.what_;
that.what_ = nullptr;
}

Error &Error::operator=(Error &&that) noexcept {
if (this != &that) {
delete[] this->what_;
this->what_ = that.what_;
that.what_ = nullptr;
}
return *this;
}

Error::~Error() {
delete[] what_;
}

const char *Error::what() const noexcept {
return what_;
}

CompileError::CompileError(const std::string &msg)
Expand All @@ -49,6 +89,18 @@ InternalError::InternalError(const std::string &msg)
: Error(msg) {
}

CompileError::CompileError(const char *msg)
: Error(msg) {
}

RuntimeError::RuntimeError(const char *msg)
: Error(msg) {
}

InternalError::InternalError(const char *msg)
: Error(msg) {
}

namespace Internal {

// Force the classes to exist, even if exceptions are off
Expand Down Expand Up @@ -96,11 +148,7 @@ ErrorReport::ErrorReport(const char *file, int line, const char *condition_strin
}
}

ErrorReport::~ErrorReport()
#if __cplusplus >= 201100 || _MSC_VER >= 1900
noexcept(false)
#endif
{
ErrorReport::~ErrorReport() noexcept(false) {
if (!msg.str().empty() && msg.str().back() != '\n') {
msg << "\n";
}
Expand Down
41 changes: 35 additions & 6 deletions src/Error.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,29 +12,58 @@ namespace Halide {
/** Query whether Halide was compiled with exceptions. */
bool exceptions_enabled();

/** A base class for Halide errors. */
struct HALIDE_EXPORT_SYMBOL Error : public std::runtime_error {
/** A base class for Halide errors.
*
* Note that this deliberately does *not* descend from std::runtime_error, or
* even std::exception; unfortunately, std::runtime_error is not marked as
* DLLEXPORT on Windows, but Error needs to be marked as such, and mismatching
* DLLEXPORT annotations in a class inheritance hierarchy in this way can lead
* to ODR violations. Instead, we just attempt to replicate the API of
* runtime_error here. */
struct HALIDE_EXPORT_SYMBOL Error {
Error() = delete;

// Give each class a non-inlined constructor so that the type
// doesn't get separately instantiated in each compilation unit.
Error(const std::string &msg);
explicit Error(const char *msg);
explicit Error(const std::string &msg);

Error(const Error &);
Error &operator=(const Error &);
Error(Error &&) noexcept;
Error &operator=(Error &&) noexcept;

virtual ~Error();

virtual const char *what() const noexcept;

private:
// Using a std::string here will cause MSVC to complain about the fact
// that class std::string isn't declared DLLEXPORT, even though the
// field is private; rather than suppress the warning, we'll just use
// an old-fashioned new-and-delete to keep it nice and clean.
char *what_;
};

/** An error that occurs while running a JIT-compiled Halide pipeline. */
struct HALIDE_EXPORT_SYMBOL RuntimeError : public Error {
RuntimeError(const std::string &msg);
explicit RuntimeError(const char *msg);
explicit RuntimeError(const std::string &msg);
};

/** An error that occurs while compiling a Halide pipeline that Halide
* attributes to a user error. */
struct HALIDE_EXPORT_SYMBOL CompileError : public Error {
CompileError(const std::string &msg);
explicit CompileError(const char *msg);
explicit CompileError(const std::string &msg);
};

/** An error that occurs while compiling a Halide pipeline that Halide
* attributes to an internal compiler bug, or to an invalid use of
* Halide's internals. */
struct HALIDE_EXPORT_SYMBOL InternalError : public Error {
InternalError(const std::string &msg);
explicit InternalError(const char *msg);
explicit InternalError(const std::string &msg);
};

/** CompileTimeErrorReporter is used at compile time (*not* runtime) when
Expand Down
8 changes: 7 additions & 1 deletion src/Generator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1044,9 +1044,15 @@ namespace Internal {
int generate_filter_main(int argc, char **argv, const GeneratorFactoryProvider &generator_factory_provider) {
try {
return generate_filter_main_inner(argc, argv, generator_factory_provider);
} catch (std::runtime_error &err) {
} catch (::Halide::Error &err) {
user_error << "Unhandled exception: " << err.what() << "\n";
return -1;
} catch (std::exception &err) {
user_error << "Unhandled exception: " << err.what() << "\n";
return -1;
} catch (...) {
user_error << "Unhandled exception: (unknown)\n";
return -1;
}
}
#else
Expand Down

0 comments on commit 92de4a1

Please sign in to comment.