util: Make logging noexcept #17514

Make the logging subsystem noexcept. This facilitates changes such as #17507 by making it possible to log in noexcept context.

First, adds a function tfm::format_noexcept that works like tfm::format for use in noexcept context if the format string and number of arguments are guaranteed to be correct (for example, if it is trivial like "%02x"). It will abort on any tinyformat error. This is used in utility functions, not for the logging strings itself, as formatting exceptions there are internally caught and logged in LogPrintf (for better troubleshooting of the complex arbitrary formatting expressions used there).

Then, go through the call hierarchy and make functions noexcept after checking them (see individual commits):

LogInstance
LogAcceptCategory
  Logger::WillLogCategory
LogPrintf
  Logger::Enabled
  LogPrintStr
    LogEscapeMessage
    util::ThreadGetInternalName()
    LogTimestampStr
      GetTimeMicros
      FormatISO8601DateTime
      GetMockTime
    fsbridge::fopen
    FileWriteStr

Though I've checked these functions carefully, it is possible that I missed a function down the call tree. Please let me know if so.

The only functions I'm slightly uncertain about are the boost posix_time functions in GetTimeMicros/GetTimeMillis, but I could not find any documented exceptions in https://www.boost.org/doc/libs/1_71_0/doc/html/date_time/posix_time.html (fixed, thanks to practicalswift)

(I know the m_print_callbacks can potentially throw, there is as far as I saw no way to guarantee std::function as noexcept. I leave this as an responsibility to who registers it. FWIW, this functionality is only used as part of the test framework.)

(Also, thinking of it, any kind of allocation in C++ can except with bad_alloc&, including inside string manipulation, which is almost 100% of the code here. If that's an issue, I'm no longer convinced this is possible at all)

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #16673 (Relog configuration args on debug.log rotation by LarryRuane)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

FYI, these are the potential tinyformat exceptions:

TINYFORMAT_ERROR("tinyformat: Cannot convert from argument type to integer for use as variable width or precision");
TINYFORMAT_ERROR("tinyformat: Not enough conversion specifiers in format string");
TINYFORMAT_ERROR("tinyformat: Not enough arguments to read variable width");
TINYFORMAT_ERROR("tinyformat: Not enough arguments to read variable precision");
TINYFORMAT_ERROR("tinyformat: the %a and %A conversion specs are not supported");
TINYFORMAT_ERROR("tinyformat: %n conversion spec not supported");
TINYFORMAT_ERROR("tinyformat: Conversion spec incorrectly terminated by end of string");
TINYFORMAT_ERROR("tinyformat: Not enough format arguments");
TINYFORMAT_ERROR("tinyformat: Too many conversion specifiers in format string");

These all boil down to:

• Wrong number of arguments in format string
• Wrong type of arguments provided (compared to argument spec)
• Malformed or unsupported format specifiers

As I understand it, assuming a static format string—anything else is very unwise, and not the case here—these can only be caused by bugs in development, not by invalid values at run-time.

So if a function with tfm::format_noexcept is at least tested once in the unit tests, it can be relied on to never abort.

I have intentionally not used tfm::format_noexcept for the log message itself, because that usage of tinyformat is much more error-prone.

Concept ACK on making logging noexcept

Some comments:

The only functions I'm slightly uncertain about are the boost posix_time functions in GetTimeMicros/GetTimeMillis, but I could not find any documented exceptions in https://www.boost.org/doc/libs/1_71_0/doc/html/date_time/posix_time.html

GetTimeMillis() does boost::posix_time::ptime(boost::gregorian::date(1970,1,1))) which can throw (from the compiler's perspective that is - we know it can't throw with these inputs :)):

• boost::gregorian::bad_year ("Year is out of valid range: 1400..10000")
• boost::gregorian::bad_month ("Month number is out of range 1..12")
• boost::gregorian::bad_day_of_month ("Day of month value is out of range 1..31")
• boost::gregorian::bad_day_of_month ("Day of month is not valid for year")

I suggest cherry-picking in https://github.com/bitcoin/bitcoin/pull/13382/commits/afc97c731e407f87fae0385aa5c3a729981ca1aa from the PR #13382 ("util: Don't throw in GetTime{Millis,Micros}(). Mark as noexcept."):

diff --git a/src/util/time.cpp b/src/util/time.cpp
index 83a7937d8f61..217bc1e9cdba 100644
--- a/src/util/time.cpp
+++ b/src/util/time.cpp
@@ -37,18 +37,20 @@ int64_t GetMockTime()
     return nMockTime.load(std::memory_order_relaxed);
 }
 
-int64_t GetTimeMillis()
+int64_t GetTimeMillis() noexcept
 {
+    static const boost::posix_time::ptime epoch = boost::posix_time::from_time_t(0);
     int64_t now = (boost::posix_time::microsec_clock::universal_time() -
-                   boost::posix_time::ptime(boost::gregorian::date(1970,1,1))).total_milliseconds();
+                   epoch).total_milliseconds();
     assert(now > 0);
     return now;
 }
 
-int64_t GetTimeMicros()
+int64_t GetTimeMicros() noexcept
 {
+    static const boost::posix_time::ptime epoch = boost::posix_time::from_time_t(0);
     int64_t now = (boost::posix_time::microsec_clock::universal_time() -
-                   boost::posix_time::ptime(boost::gregorian::date(1970,1,1))).total_microseconds();
+                   epoch).total_microseconds();
     assert(now > 0);
     return now;
 }
diff --git a/src/util/time.h b/src/util/time.h
index f2e27474342b..ae618165063a 100644
--- a/src/util/time.h
+++ b/src/util/time.h
@@ -20,8 +20,8 @@
  */
 
 int64_t GetTime();
-int64_t GetTimeMillis();
-int64_t GetTimeMicros();
+int64_t GetTimeMillis() noexcept;
+int64_t GetTimeMicros() noexcept;
 int64_t GetSystemTimeInSeconds(); // Like GetTime(), but not mockable
 void SetMockTime(int64_t nMockTimeIn);
 int64_t GetMockTime();

(Also, thinking of it, any kind of allocation in C++ can except with bad_alloc&, including inside string manipulation, which is almost 100% of the code here. If that's an issue, I'm no longer convinced this is possible at all)

I don't think we have to worry about bad_alloc. From the C++ Core Guidelines (with editors Bjarne Stroustrup & Herb Sutter):

"Code that is intended to be perfectly general (like the standard library and other utility code of that sort) needs to support environments where a bad_alloc exception may be handled meaningfully. However, most programs and execution environments cannot meaningfully handle a failure to allocate, and aborting the program is the cleanest and simplest response to an allocation failure in those cases. If you know that your application code cannot respond to an allocation failure, it may be appropriate to add noexcept even on functions that allocate."

GetTimeMillis() does boost::posix_time::ptime(boost::gregorian::date(1970,1,1))) which can throw:

Oh crap. Thanks for noticing. Though, I guess that will never happen with this static input. But I think your commit improves the code anyway.

Thanks, will cherry-pick that!

Edit: replaced my commit

Concept ACK

It will abort on any tinyformat error.

Is this still the case? I'm testing 71493f0a55f6fa0a23e00d9d239aa6d8210d89ab with this diff:

     RandAddStaticEnv(hasher);
-    LogPrintf("Feeding %i bytes of environment data into RNG\n", hasher.Size() - old_size);
+    LogPrintf("Feeding %a bytes of environment data into RNG\n", hasher.Size() - old_size);

which triggers this TINYFORMAT_ERROR: https://github.com/bitcoin/bitcoin/blob/d8a66626d63135fd245d5afc524b88b9a94d208b/src/tinyformat.h#L750-L751

bitcoind just prints the error and continues:

2019-11-27T14:49:55Z Error "tinyformat: the %a and %A conversion specs are not supported" while formatting log message: Feeding %i %a bytes ....

The same thing also happens with 3c8505ca49a429dcc6c79fc3fd5d1e752b2f22e8.

Is this still the case? I'm testing 71493f0 with this diff:

No, not in general. strprintfstill raises tinyformat_error. LogPrintf's behavior for invalid format strings is unchanged (the exception is caught and reported, but does not leak out)

Note that the paragraph related to the use of tfm::format_noexcept specifically. I even mention in the second post

I have intentionally not used tfm::format_noexcept for the log message itself, because that usage of tinyformat is much more error-prone.

What this does is make LogPrintf and various utility functions noexcept (no exceptions are guaranteed to leak out). Nothing more. No observable behavior is supposed to change.

I'm not entirely sure this is worth doing anymore. noexcept works slightly different from what I thought when I wrote this PR. it seems it's perfectly fine to use non-noexcept functions from except context if you expect they don't ever except anyway. I'm not sure what this adds at all. so this is not a requirement for #17507 at all

@laanwj

I think it is worth doing.

What this adds is a guarantee that logging won't throw any exceptions. That enables the compiler to apply optimisations that otherwise would not be possible (by reducing the number of alternative execution paths), and it also simplifies control flow analysis from the perspective of static analysers and human reviewers.

This is why Scott Mayer of "Effective C++" fame recommends using noexcept in cases like the one we are discussing:

The difference between unwinding the call stack and possibly unwinding it has a surprisingly large impact on code generation. In a noexcept function, optimizers need not keep the runtime stack in an unwindable state if an exception would propagate out of the function, nor must they ensure that objects in a noexcept function are destroyed in the inverse order of construction should an exception leave the function.

The result is more opportunities for optimization, not only within the body of a noexcept function, but also at sites where the function is called. Such flexibility is present only for noexcept functions. Functions with throw() exception specifications lack it, as do functions with no exception specification at all. The situation can be summarized this way:

RetType function(params) noexcept; // most optimizable
RetType function(params) throw(); // less optimizable
RetType function(params); // less optimizable

This alone should provide sufficient motivation to declare functions noexcept whenever you can.

[…]

Things to Remember

• noexcept is part of a function’s interface, so callers may depend on it.
• noexcept functions are more optimizable than non-noexcept functions.
• noexcept is particularly valuable for the move operations and for swap.
• Most functions are exception-neutral rather than noexcept

Since logging is used pretty much everywhere in our code base I think it would be good to add a noexcept guarantee to remove the "potentially throwing" effect that using logging currently has on all call sites (and by extension all call sites directly or indirectly calling said call sites: it bubbles up :)).

I've noticed that @sipa is a actively introducing noexcept where appropriate: perhaps he might want to chime in :)

Squashed the fixup commit and rebased.