[SERVER-70148] Document proper noexcept usage Created: 30/Sep/22  Updated: 29/Jan/24

Status: Open
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Andrew Witten (Inactive) Assignee: Backlog - Service Architecture
Resolution: Unresolved Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
is related to SERVER-50434 exceptions violating noexcept can be ... Backlog
Assigned Teams:
Service Arch
Operating System: ALL
Sprint: Service Arch Prioritized List, Service Arch 2023-08-21, Service Arch 2023-09-04, Service Arch 2023-09-18, Service Arch 2023-10-02, Service Arch 2023-10-16, Service Arch 2023-10-30, Service Arch 2023-11-13, Service Arch 2023-11-27, Service Arch 2023-12-11, Service Arch 2023-12-25, Service Arch 2024-01-08, Service Arch 2024-01-22
Participants:

 Description   

I think SERVER-50434 is a big deal.  This gcc bug means that when we violate a noexcept specification, we have very few tools for figuring out where it came from.  This happened in BF-26507 (an ongoing investigation) and in BF-17935.  We get no log messages saying what exception was thrown.  And in the case of BF-26507 (and possibly BF-17935), inlining makes the backtrace unhelpful.

 

In most of the cases where we use noexcept for "exception handling," we would be better off catching the exception, logging a message, and calling std::terminate.

 

I'm not sure how actionable this ticket is (feel free to close it as a duplicate of SERVER-50434), but I just want to bring people's attention to it.

 



 Comments   
Comment by Jason Chan [ 13/Jun/23 ]

billy.donahue@mongodb.com's suggestion that a minimal effort improvement would be to document about noexcept and its recommended usage patterns in the architecture guide.

Comment by Billy Donahue [ 07/Oct/22 ]

We'd never ban it.
We'd have to consider it with some nuance.

noexcept is still vitally necessary for move operations, swaps, and things that really and truly won't actually throw.
It serves as a queryable hint for callers to choose more efficient exception-safe algorithms.

We just can't rely on the compiler to do the right thing when it's violated, is the thing.

Noexcept does a few things, not all of which are broken.

(1) It publishes a compiler-visible interface to a caller guaranteeing that the caller doesn't have to handle exceptions as a return path of a call. This allows algorithm selection and very theoretically some codegen optimization (particularly if the callee is not inline).

(2) It sets up an implicit std::terminate try block around the body of the function with possible implementation-defined unwind elision.

(3) A form of documentation.

We would be using it SOLELY as a way of augmenting a function's signature, as in (1).
Feature (2) is hosed. If you want to make a nontrivial function noexcept for some reason, you could write your own try{ stuff }catch(...){std::terminate());} block around it and not rely on (2).

It's much simpler to just not use noexcept at all for functions besides 1-ish liners.

As for (3), I don't know how hot a take this is, but I don't think it's a good idea to use this keyword in lieu of docs. It doesn't allow future refinement of the API and noexcept is a forever kind of API decision that's unnecessary. It's also misused in our codebase to say that a function doesn't happen to throw, which is more a synopsis of implementation detail than a statement of contract. I think it can be harmful to readers because they don't know which meaning of noexcept is implied when they see it in code or are investigating BFs related to noexcept violations.

Comment by Billy Donahue [ 05/Oct/22 ]

The underlying GCC bug is "UNCONFIRMED" upstream https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97720
I haven't seen it in a v4 toolchain build directly because most of our builds aren't v4 toolchain, but GCC hasn't acted on it.
It's possible (saying this without searching for evidence to the contrary) the v4 toolchain GCC's optimizer doesn't have or at least doesn't manifest this bug anymore for whatever reason, but we haven't had a great repro for it.

Even the original GCC bugzilla bug report was sure to point out how dependent this behavior is on the exact arrangement of source code and compiler versions. In practice, we see that most of our terminate handler invocations do indeed get a current_exception, but it looks like it's possible for the wind to change direction and have that not be the case anymore for some failure paths.

Comment by Henrik Edin [ 05/Oct/22 ]

Do we still have this GCC bug in the v4 toolchain?

Comment by Billy Donahue [ 03/Oct/22 ]

-Wexceptions, being a static check, will only catch trivial cases where a throw occurs directly inside a noexcept function. It's not a substitute for the action proposed by this ticket.

noexcept is good and necessary for move operations, without a doubt. As for performance, yes that's a consideration, but I'd say only for the most trivial and hottest of functions, like a trivial getter or other operators. Inlining these functions produces the same benefit.

This ticket is meant primarily to work around a GCC bug that makes noexcept violations impossible to track down, in cases where a simple uncaught exception would be traceable.

A noexcept mark is not a form of documentation. This trend has thankfully been halted, I believe.

We have made a lot of progress toward backing out of unnecessary noexcepts over the last few years and should not be changing course without a serious proposal to do so.

Lakos et al "Embracing Modern C++ Safely", after a lengthy chapter on noexcept, categorizes it as an "unsafe" feature, a category for features that have conditional utility, for sure, but a significant potential for causing harm on a large codebase through copycat overuse. A lot of thought has gone into this topic.
https://www.cppstories.com/2022/embracing-modern-cpp-book/

Comment by Alex Neben [ 03/Oct/22 ]

I think the better solution would to add a compiler warning when throwing in a noexcept: https://clang.llvm.org/docs/DiagnosticsReference.html#wexceptions . There are good and legitimate cases for using noexcept for performance and clarity reasons.

Generated at Thu Feb 08 06:15:24 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.