[libc++] Add a helper class to write code with the strong exception guarantee

[ldionne]

Summary

__transaction is a helper class that allows rolling back code in case an
exception is thrown. The main goal is to reduce the clutter when code
needs to be guarded with #if _LIBCPP_NO_EXCEPTIONS.

[• Quuxplusone]

libcxx/include/__detail/transaction.h

1

I suggest that this should go in __utility/ for now — alongside priority_tag and (D115686 auto_cast) and decay_copy and to_underlying and so on.
I can imagine a direction where we take all those "truly internal internals" and move them all into __detail/ or __misc/ or something, to indicate that they're intentionally not directly included by any standard header... but I don't see a compelling reason for this one facility to be inconsistent with those others for now.

Also, putting it in <utility> sidesteps issues such as

• Can we have a detail header without a top-level module?
• If this detail header is its own top-level module, that breaks the invariant that detail headers are never included directly (see: your new test)
• Bikeshedding the name of __detail/

20

If this facility is useful, it might be useful in C++11/14 as well. I think it's worth thinking now about how it could be enabled for C++11.

31

I suggest eliminating the move-constructor/assignment, since I can't think of any place it would be useful. Passing a transaction up or down the call stack seems like an antipattern: it's designed to replace try/catch, which is lexically, not dynamically, scoped. I'd make it non-movable, like mutex.

52

This default-constructs the _Rollback and leaves random garbage in __completed_, so this is bad.
I'm confident that this class should not be default-constructible.

libcxx/test/libcxx/detail/transaction.pass.cpp

125

I'd also like to see that it works properly when called from a destructor. (This is more obviously a big deal for my alternative __on_exception_rollback design, but might as well check it no matter which design we pick.)

bool rolled_back = false;
struct S {
    ~S() {
        std::__transaction t([&]{ rolled_back = true; });
        t.__set_completed();
    }
};
try {
    S s;
    throw 0;
} catch (...) {
    assert(!rolled_back);
}

(I'd say if S has a data member of type __transaction, that produces weird behavior but also violates the intended usage of the type, and so we shouldn't worry about that case.)

[Mordante]

In general LGTM, some small nits and questions.

libcxx/include/CMakeLists.txt

143

Just curious what is the __detail directory intended to contain?

libcxx/include/__detail/transaction.h

48

I like this comment a lot!

51

Somewhat curious, did you consider implementing scope_exit instead of __transaction?
https://en.cppreference.com/w/cpp/experimental/scope_exit/scope_exit

(I know that isn't constexpr (yet))

60

60

Should this function be conditionally noexcept?

75

[var-const]

libcxx/include/__detail/transaction.h

20

It seems like this would significantly limit the scope where __transaction can be used internally. From a quick glance, it seems like [[no_unique_address]] can be replaced by EBO and the rest of the class should pretty much work for all language modes that support constexpr, with minor adjustments.

31

I would also add that adding move support later on is a straightforward extension, unlike the other way round.

55

Question: would it make sense to create both (_Rollback&&) and (const _Rollback&) to avoid an extra move?

75

Nit: because this class only works in C++20 and above, it seems like this could be just constexpr. However, if you plan to make it available for earlier language versions, then it's not worth spending the time changing this.

81–82

Hmm, it kinda looks like an optional<_Rollback>, but going that route would make it hard to have this class available in older language modes.

libcxx/test/libcxx/detail/transaction.pass.cpp

65

Question: would it make sense to also check sizeof to make sure the [[no_unique_address]] optimization works as expected?

[ldionne]

Address review comments.

libcxx/include/CMakeLists.txt

143

It was meant to contain implementation details of the library that don't really fit in something else like __memory/, __algorithm/ or something like that. But per Arthur's comment, I will delay having that discussion to later.

libcxx/include/__detail/transaction.h

1

This is not the first time it happens :-). I had originally put it in __utility/ and then I moved it to __detail because I found it weird to include it in <utility>. But yeah, I agree with your pragmatic approach. I do think we will want a true directory for internal implementation details, but it's easy to create that later.

20

Yup, agreed, will adjust it. I've got the tests working in C++11 and above. Porting them to C++03 is just too painful cause we can't use lambdas at all, so I'll make an effort to make __transaction work in C++03, but I won't port the test. LMK if you're not OK with that.

Regarding [[no_unique_address]], I actually suggest removing it altogether if we want to support this in prior standards. Concretely, we will almost always capture some state in the lambda to perform the rollback, so there would be no optimization here. Furthermore, this isn't the kind of type that we will store and have many instances of, and so IMO optimizing the storage is not as important as e.g. std::string or std::optional.

31

I don't have an extremely strong preference to keep move construction (move assignment is already disabled), however IMO we'd be artificially limiting the convenience of the class for little reason. For now I've added tests for noexcept-ness and so on, but let me know if you feel strongly that the class would be better without being move constructible and I can change that. My rationale for keeping it would go along the lines of "It's implemented, tested and it matches what experimental::scope_exit does" -- but I don't feel strongly about it.

48

Thank you!

51

I had not. However, since __transaction is for internal use and we can't use experimental APIs internally (cause those go away eventually), I think it makes sense to keep those separate.

52

My thinking was that it might be useful in case we wanted to define rollback instructions in a stateless function object, not as a lambda. However, since we will usually need to capture some state in the function we're guarding, default construction should rarely (if ever) be useful. Will remove it for now, and we can add it later if there is a use case.

55

I thought about it, and decided not to do it because this should generally be initialized by a cheap-to-move (and even cheap-to-copy) function object. I don't think it will ever matter, but if you think it may, let me know and I'll turn this into two overloads.

75

Yeah, I was kind of on the fence when I wrote it, but I'll make it work in all standard modes, so this is going to stay.

81–82

Also, IMO the class is so tiny it makes sense to just keep the bool there -- we wouldn't be simplifying much by using optional.

libcxx/test/libcxx/detail/transaction.pass.cpp

65

See my comment above, but I suggest removing [[no_unique_address]] altogether if we support older standard modes. Otherwise, yes I think it would have made sense.

LMK if you disagree with the decision to remove [[no_unique_address]].

125

Adding the test.

Sorry, what does __on_exception_rollback do? Does it check whether the destructor is being called during stack unwinding?

[var-const]

libcxx/include/__detail/transaction.h

55

I think I have a very slight preference towards doing two overloads. My reasoning is that this helper class should ideally add no performance penalty (and minimal performance penalty in non-optimized builds), and it being low/no-overhead makes it more attractive for internal usage. A combinatorial explosion of overloads seems very unlikely, and two overloads I think are tolerable.

Then again, I might be overly cautious about performance issues here. I don't feel very strongly about this and can certainly be convinced that this isn't important in this case.

81–82

I think optional would also give EBO, but given the discussion above, this isn't important.

[DanielMcIntosh-IBM]

libcxx/include/__utility/transaction.h

80 ↗

(On Diff #394392)

Would it be possible to get rid of __complete() by replacing this with something like if (std::uncaught_exceptions() > initial_exception_count) and initializing initial_exception_count = std::uncaught_exceptions() in the constructor?

[• Quuxplusone]

libcxx/test/libcxx/detail/transaction.pass.cpp

125

Yeah, exactly what @DanielMcIntosh-IBM just said elsewhere in this review. :)
https://godbolt.org/z/78vrfj8e7
The bad thing about uncaught_exceptions is that compilers don't understand that its value never changes by ordinary means, so when the __on_exception_rollback object is destroyed in the ordinary way at the end of the scope, it causes codegen. Actually, that might make __on_exception_rollback a complete non-starter, unfortunately. We can't have something in a random library scope that unconditionally adds two calls to std::uncaught_exceptions on the success path. :(

However, taking another page from the Auto.h playbook, I think you should consider eliminating the completed_ member altogether and just making the lambda always run on destruction. If the programmer wants the lambda's behavior to be conditional, they can just... put an if in it.

bool completed = false;
__on_scope_exit guard([&]() {
    if (!completed) puts("Rolling back!");
});
puts("Starting transaction");
f();
completed = true;
// guard runs here

IMO, this is a more readable API than having a magic .set_completed() method on the guard object itself.

[Mordante]

libcxx/include/__utility/transaction.h

52 ↗

(On Diff #394392)

Why not omit this?

55 ↗

(On Diff #394392)

I see a lot of different constexpr macros in this class
_LIBCPP_CONSTEXPR, _LIBCPP_CONSTEXPR_AFTER_CXX11 and _LIBCPP_CONSTEXPR_AFTER_CXX17. Looking at this I expect the class not be usable as constexpr before C++20, due to the constexpr macro for the destructor.

62 ↗

(On Diff #394392)

The tests require C++11 or later.

libcxx/test/libcxx/utilities/transaction.pass.cpp

155 ↗

(On Diff #394392)

Please make sure this version matches the version in which the class is intended to be constexpr.

[• Quuxplusone]

libcxx/include/__utility/transaction.h

55 ↗

(On Diff #394392)

This is presumably due to a style guideline that I made up ;) a while ago: "use the most aggressive constexpr macro possible for each place in internal code," so that if we ever drop support for C++03 (or Clang gains support for constexpr as an extension in C++03 mode), we can replace the absolute maximum number of uses of _LIBCPP_CONSTEXPR in one fell swoop. I.e. I think this is fine.

But, given the intended usage of this class... do we need it to be constexpr at all? Certainly not for call_once or uninitialized_copy_n. @ldionne, could you add some sample usages (in places like <algorithm> and the various containers, maybe) that show why constexprness is a good idea? Or if no great examples are conspicuous, I'd recommend removing the constexpr keywords entirely for now.

[Mordante]

libcxx/include/__utility/transaction.h

55 ↗

(On Diff #394392)

I wasn't aware of this new style and haven't noticed it before during reviews. To me it feels odd especially when the destructor is the "limiting factor". To be honest I personally don't like this style.

That being said, if this is the style we've used before, then I withdraw my earlier comment about mixing the macros.

[• Quuxplusone]

libcxx/include/__utility/transaction.h

55 ↗

(On Diff #394392)

The rule makes a lot more sense (IMO) when it's talking about some random free function, like e.g. __sift_up or __sort3 (constexpr-after-11, even though their only callers are constexpr-after-17). I'll gladly agree that a class with a bunch of member functions at different levels is a pretty pathological case for this rule (although I'll be at-best-ambivalent as to whether that means we should abandon the rule even in this pathological case).

[ldionne]

Fix constexpr macros

libcxx/include/__utility/transaction.h

52 ↗

(On Diff #394392)

I like calling out explicitly the properties of types instead of relying too much on the implicit rules, because these rules are pretty complicated. For example, see how I explicitly delete the copy constructor and the assignment operators below -- I could have relied on any number of implicit rules (e.g. the fact that we define a destructor explicitly) to get the same effect, but IMO it forces the reader to be more cleaver than I want.

LMK if you are not convinced and I'll remove it.

55 ↗

(On Diff #394392)

My original intent was to make things constexpr as early as possible, but I agree this ends up being somewhat messy. Instead, I've made everything be constexpr in C++20 and above only.

The benefit of making this class constexpr is that it can be used inside functions that are themselves constexpr (there's more than just the uninitialized_foo family that will benefit from this). If we want to use it in a C++11/C++14/C++17 constexpr function, we can come back and try to make it work.

@Quuxplusone An example would be std::vector::insert. Like I said, we can come back later and make it constexpr friendly in older standards if we have a use for it, but I wouldn't de-constexprify this upfront.

62 ↗

(On Diff #394392)

The tests do, however we're still going to use the class in places that are supported in C++03, like some algorithms, so I can't use raw noexcept.

80 ↗

(On Diff #394392)

Yes, however as Arthur explained in another comment (https://reviews.llvm.org/D115730?id=394248#inline-1106466), we don't want to add a call to uncaught_exceptions() on the success path. The benefit of the current approach is that the compiler will be able to see that we call __complete() on all happy paths, and as a result it should be able to do optimal codegen.

[Mordante]

LGTM!

libcxx/include/__utility/transaction.h

52 ↗

(On Diff #394392)

Actually I tend to do the same for copy/move constructor/assignment for the same reasons.
Somehow I never felt the need for the same needs for the default constructor.

So let's keep it.

[ldionne]

I'll land this -- please leave any comments here and I can apply them post-commit if needed.

[• Quuxplusone]

@ldionne: I'd still prefer the version I showed in https://reviews.llvm.org/D115730?id=394248#inline-1106466 where the object doesn't have a completed data member at all (so it would better be named __on_scope_exit rather than __transaction). I'd also be interested in seeing a godbolt or something showing which approach gives better codegen. Naïvely, I would expect my preferred version would be no worse, and maybe a tiny bit better, because it gives the optimizer more freedom to rearrange the stack frame. But the reason I prefer it is for simplicity/readability: I think the set_completed/__complete thing is easy to mess up.

[ldionne]

In D115730#3203479, @Quuxplusone wrote:

@ldionne: I'd still prefer the version I showed in https://reviews.llvm.org/D115730?id=394248#inline-1106466 where the object doesn't have a completed data member at all (so it would better be named __on_scope_exit rather than __transaction). I'd also be interested in seeing a godbolt or something showing which approach gives better codegen. Naïvely, I would expect my preferred version would be no worse, and maybe a tiny bit better, because it gives the optimizer more freedom to rearrange the stack frame. But the reason I prefer it is for simplicity/readability: I think the set_completed/__complete thing is easy to mess up.

I have a strong distaste for managing the bool completed and having to enclose all the rollback code in if (!completed). IMO that's a lot easier to mess up than just making sure a __complete() method is called. Since this simple helper is really tailored for exception handling (it's purely a means to remove #if _LIBCPP_NO_EXCEPTIONS), I'd rather keep as-is.

libcxx/include/CMakeLists.txt

set(files
   __concepts/swappable.h
   __concepts/totally_ordered.h
   __config
   __coroutine/coroutine_handle.h
   __coroutine/coroutine_traits.h
   __coroutine/noop_coroutine_handle.h
   __coroutine/trivial_awaitables.h
   __debug
+  __detail/transaction.h
   __errc
   __format/format_arg.h
   __format/format_args.h
   __format/format_context.h
   __format/format_error.h
   __format/format_fwd.h
   __format/format_parse_context.h
   __format/format_string.h

libcxx/include/__detail/transaction.h

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___DETAIL_TRANSACTION_H
+#define _LIBCPP___DETAIL_TRANSACTION_H
+
+#include <__config>
+#include <__utility/exchange.h>
+#include <__utility/move.h>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#pragma GCC system_header
+#endif
+
+#if _LIBCPP_STD_VER > 17
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+// __transaction is a helper class for writing code with the strong exception guarantee.
+//
+// When writing code that can throw an exception, one can store rollback instructions in a
+// transaction so that if an exception is thrown at any point during the lifetime of the
+// transaction, it will be rolled back automatically. When the transaction is done, one
+// must mark it as being complete so it isn't rolled back when the transaction is destroyed.
+//
+// Transactions can't be copied or assigned to, but they can be moved around for convenience.
+//
+// __transaction can help greatly simplify code that would normally be cluttered by
+// `#if _LIBCPP_NO_EXCEPTIONS`. For example:
+//
+//    template <class Iterator, class Size, class OutputIterator>
+//    Iterator uninitialized_copy_n(Iterator iter, Size n, OutputIterator out) {
+//        typedef typename iterator_traits<Iterator>::value_type value_type;
+//        __transaction transaction([start=out, &out] {
+//            std::destroy(start, out);
+//        });
+//
+//        for (; n > 0; ++iter, ++out, --n) {
+//            ::new ((void*)std::addressof(*out)) value_type(*iter);
+//        }
+//        transaction.__complete();
+//        return out;
+//    }
+//
+template <class _Rollback>
+struct __transaction {
+    __transaction() = default;
+
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr explicit __transaction(_Rollback __rollback)
+        : __rollback_(_VSTD::move(__rollback))
+        , __completed_(false)
+    { }
+
+    constexpr __transaction(__transaction&& __other)
+        : __rollback_(_VSTD::move(__other.__rollback_))
+        , __completed_(_VSTD::exchange(__other.__completed_, true))
+    { }
+
+    __transaction(__transaction const&) = delete;
+    __transaction& operator=(__transaction const&) = delete;
+    __transaction& operator=(__transaction&&) = delete;
+
+    _LIBCPP_HIDE_FROM_ABI
+    constexpr void __complete() {
+        __completed_ = true;
+    }
+
+    _LIBCPP_HIDE_FROM_ABI
+    _LIBCPP_CONSTEXPR_AFTER_CXX17 ~__transaction() {
+        if (!__completed_)
+            __rollback_();
+    }
+
+private:
+    [[no_unique_address]] _Rollback __rollback_;
+    bool __completed_;
+};
+
+_LIBCPP_END_NAMESPACE_STD
+
+#endif // _LIBCPP_STD_VER > 17
+
+#endif // _LIBCPP___DETAIL_TRANSACTION_H

libcxx/include/module.modulemap

module __coroutine {
       module noop_coroutine_handle      { private header "__coroutine/noop_coroutine_handle.h" }
     }
   }
   module deque {
     header "deque"
     export initializer_list
     export *
   }
+
+  module __detail {
+    private header "__detail/transaction.h"
+  }
+
   module exception {
     header "exception"
     export *
   }
   module execution {
     header "execution"
     export *
   }

libcxx/test/libcxx/detail/transaction.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
+#include <__detail/transaction.h>
+#include <cassert>
+#include <type_traits>
+#include <utility>
+
+#include "test_macros.h"
+
+constexpr bool test() {
+    // Make sure the transaction is rolled back if it is not marked as complete when
+    // it goes out of scope.
+    {
+        bool rolled_back = false;
+        {
+            std::__transaction t([&] { rolled_back = true; });
+        }
+        assert(rolled_back);
+    }
+
+    // Make sure the transaction is not rolled back if it is marked as complete when
+    // it goes out of scope.
+    {
+        bool rolled_back = false;
+        {
+            std::__transaction t([&] { rolled_back = true; });
+            t.__complete();
+        }
+        assert(!rolled_back);
+    }
+
+    // Make sure that we will perform the right number of rollbacks when a transaction has
+    // been moved around
+    {
+        // When we don't complete it (exactly 1 rollback should happen)
+        {
+            int rollbacks = 0;
+            {
+                std::__transaction t([&] { ++rollbacks; });
+                auto other = std::move(t);
+            }
+            assert(rollbacks == 1);
+        }
+
+        // When we do complete it (no rollbacks should happen)
+        {
+            int rollbacks = 0;
+            {
+                std::__transaction t([&] { ++rollbacks; });
+                auto other = std::move(t);
+                other.__complete();
+            }
+            assert(rollbacks == 0);
+        }
+    }
+
+    // Basic properties of the type
+    {
+        struct Rollback { constexpr void operator()() const { } };
+        using Transaction = std::__transaction<Rollback>;
+
+        static_assert(std::is_default_constructible_v<Transaction>);
+
+        static_assert(!std::is_copy_constructible_v<Transaction>);
+        static_assert(std::is_move_constructible_v<Transaction>);
+
+        static_assert(!std::is_copy_assignable_v<Transaction>);
+        static_assert(!std::is_move_assignable_v<Transaction>);
+
+        // Make sure a transaction is not default constructible if its rollback
+        // operation is not default constructible.
+        {
+            struct NoDefaultRollback {
+                NoDefaultRollback() = delete;
+                constexpr void operator()() const { }
+            };
+            using NoDefaultTransaction = std::__transaction<NoDefaultRollback>;
+            static_assert(!std::is_default_constructible_v<NoDefaultTransaction>);
+        }
+    }
+
+    return true;
+}
+
+void test_exceptions() {
+#ifndef TEST_HAS_NO_EXCEPTIONS
+    // Make sure the rollback is performed when an exception is thrown during the
+    // lifetime of the transaction.
+    {
+        bool rolled_back = false;
+        try {
+            std::__transaction t([&] { rolled_back = true; });
+            throw 0;
+        } catch (...) { }
+        assert(rolled_back);
+    }
+
+    // Make sure we don't roll back if an exception is thrown but the transaction
+    // has been marked as complete when that happens.
+    {
+        bool rolled_back = false;
+        try {
+            std::__transaction t([&] { rolled_back = true; });
+            t.__complete();
+            throw 0;
+        } catch (...) { }
+        assert(!rolled_back);
+    }
+#endif // TEST_HAS_NO_EXCEPTIONS
+}
+
+int main(int, char**) {
+    test();
+    test_exceptions();
+    static_assert(test());
+    return 0;
+}

libcxx/test/libcxx/diagnostics/detail.headers/detail/transaction.module.verify.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: modules-build
+
+// WARNING: This test was generated by 'generate_private_header_tests.py'
+// and should not be edited manually.
+
+// expected-error@*:* {{use of private header from outside its module: '__detail/transaction.h'}}
+#include <__detail/transaction.h>