Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error handling in an -fno-exceptions world #3418

Closed
brevzin opened this issue May 3, 2023 · 2 comments · Fixed by #3439
Closed

Error handling in an -fno-exceptions world #3418

brevzin opened this issue May 3, 2023 · 2 comments · Fixed by #3439

Comments

@brevzin
Copy link
Contributor

brevzin commented May 3, 2023

error_handler::on_error does this:

fmt/include/fmt/core.h

Lines 635 to 638 in d8973bf

// This function is intentionally not constexpr to give a compile-time error.
FMT_NORETURN void on_error(const char* message) {
throw_format_error(message);
}

Which hides the throwing behavior behind a macro:

FMT_FUNC void throw_format_error(const char* message) {
FMT_THROW(format_error(message));
}

Which conditionally either throws or asserts:

fmt/include/fmt/format.h

Lines 105 to 128 in d8973bf

#ifndef FMT_THROW
# if FMT_EXCEPTIONS
# if FMT_MSC_VERSION || defined(__NVCC__)
FMT_BEGIN_NAMESPACE
namespace detail {
template <typename Exception> inline void do_throw(const Exception& x) {
// Silence unreachable code warnings in MSVC and NVCC because these
// are nearly impossible to fix in a generic code.
volatile bool b = true;
if (b) throw x;
}
} // namespace detail
FMT_END_NAMESPACE
# define FMT_THROW(x) detail::do_throw(x)
# else
# define FMT_THROW(x) throw x
# endif
# else
# define FMT_THROW(x) \
do { \
FMT_ASSERT(false, (x).what()); \
} while (false)
# endif
#endif

But asserting... might not assert:

fmt/include/fmt/core.h

Lines 330 to 341 in d8973bf

#ifndef FMT_ASSERT
# ifdef NDEBUG
// FMT_ASSERT is not empty to avoid -Wempty-body.
# define FMT_ASSERT(condition, message) \
fmt::detail::ignore_unused((condition), (message))
# else
# define FMT_ASSERT(condition, message) \
((condition) /* void() fails with -Winvalid-constexpr on clang 4.0.1 */ \
? (void)0 \
: fmt::detail::assert_fail(__FILE__, __LINE__, (message)))
# endif
#endif

What this means is that a program like:

#include <fmt/format.h>

int main() {
    fmt::println(fmt::runtime("Hi {:d}"), "10");
}

could:

  1. If exceptions are enabled, throw an exception for bad format specifier.
  2. Otherwise, if NDEBUG isn't defined, terminate after printing an error about bad format specifier.
  3. Otherwise (if -fno-exceptions -DNDEBUG), just... keep... going?

So the above program could just print "Hi 10". That's pretty weird. Also we're violating the [[noreturn]] on throw_format_error since in this case it actually returns.

Maybe we should introduce a FMT_ALWAYS_ASSERT that always does the (condition) ? (void)0 : assert_fail(~) bit, and have FMT_THROW fallback to FMT_ALWAYS_ASSERT instead of FMT_ASSERT?

@sunmy2019
Copy link
Contributor

Maybe we should introduce a FMT_ALWAYS_ASSERT that always does the (condition) ? (void)0 : assert_fail(~) bit, and have FMT_THROW fallback to FMT_ALWAYS_ASSERT instead of FMT_ASSERT?

makes sense. and actually

 #    define FMT_THROW(x)               \ 
       do {                             \ 
         fmt::detail::assert_fail(__FILE__, __LINE__, x.what()); \ 
       } while (false)
 #  endif 

seems better

@vitaut
Copy link
Contributor

vitaut commented May 15, 2023

But asserting... might not assert

Well, that's how assert works =).

I don't think we need FMT_ALWAYS_ASSERT but replacing assert with direct invocation of fmt::detail::assert_fail in that particular case makes sense. A PR is welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants