util/check.h Assert/Assume: namespacing issues #24654

1. 
   ajtowns commented at 2:45 am on March 24, 2022: member

   The Assert/Assume macros are implemented via lambda functions to allow the result of the assertion to both be evaluated (and trigger an abort) and be returned without having the expression be evaluated twice.

   This causes some ugly namespacing issues though. Because there’s a function call, clang’s thread safety annotations don’t get passed through (as the lambda is unannotated), possibly causing an unnecessary compiler error because the compiler loses track that a mutex is held when a guarded variable is accessed.

   It also seems that gcc (but not clang) gets confused about member functions (but not member variables), eg:

    0class TestAssert
    1{
    2public:
    3    int variable = 3;
    4    int test_1(void) { return variable; }
    5    int test_2(void) {
    6        auto x = [&]() {
    7            Assert(test_1() == 3);
    8        };
    9        x();
   10        return ++variable;
   11    }
   12};
   

   results in:

   0test/util_tests.cpp:91:26: error: cannot call member function ‘int util_tests::TestAssert::test_1()’ without object
   1   91 |             Assert(test_1() == 3);
   

   requiring you to write Assert(this->test_1() == 3) instead.

2. ajtowns added the label Feature on Mar 24, 2022
3. 
   ajtowns commented at 2:50 am on March 24, 2022: member

   Changing the Assert macro to something like:

    0void assertion_fail(const char* file, int line, const char* func, const char* assertion)
    1{
    2    auto str = strprintf("%s:%s %s: Assertion `%s' failed.\n", file, line, func, assertion);
    3    fwrite(str.data(), 1, str.size(), stderr);
    4    std::abort();
    5}
    6
    7template <typename T>
    8inline T inline_assertion_check(T&& val, const char* file, int line, const char* func, const char* assertion)
    9{
   10    if (!val) assertion_fail(file, line, func, assertion);
   11    return std::forward<T>(val);
   12}
   13
   14#define Assert(val) inline_assertion_check(val, __FILE__, __LINE__, __func__, #val)
   

   might work to avoid this – that way val is evaluated in the original context where all the locks and this pointers are already known about. I’m not sure if this handles all C++’s various copy/move rules as well as the current Assert does though. It also gives up on re-using assert() and just manually reports file/line/function.

4. 
   sipa commented at 3:03 am on March 24, 2022: member

   @ajtowns I think the return type should be T&& for the forwarding to work.
5. 
   MarcoFalke commented at 6:06 am on March 24, 2022: member

   Related: #21596
6. 
   ajtowns commented at 1:53 pm on March 25, 2022: member

   @sipa I can’t convince myself that returning T&& wouldn’t mess up lifetimes somehow and return a dangling reference… Trying it out, it seems like T as the return-type might invoke the move constructor while T&& might not, so what you say at least seems plausible. I think the current approach (with a lambda) also invokes the move constructor; though destruction happens in a different order…
7. 
   sipa commented at 3:59 pm on March 25, 2022: member

   @ajtowns I’m happy to reason through a specific case you’re concerned about if you have one. But perhaps this helps: std::forward<T> has as return type also T&& (note that (A&)&& = A&, so if T=A&, the return type is A&, not A&&).
8. 
   ajtowns commented at 5:20 pm on March 25, 2022: member

   @sipa I think my concern is that f(1+2) + 4 might be interpreted as “pass 3 to f, then destruct 3, then add 4 to whatever f returned” which would cause a problem if f were returning the 3 object that was just destructed. But I think “All temporary objects are destroyed as the last step in evaluating the full-expression that (lexically) contains the point where they were created,” means that’s not allowed?
9. 
   sipa commented at 5:34 pm on March 25, 2022: member

   @ajtowns So I think the only concern in that regard is something like:

   0auto&& x = f(1+2);
   1return x + 4;
   

   where the x is a dangling reference to the temporary created by 1 + 2.

   However, that’s just as dangerous as doing the same with auto&& x = std::forward<int>(1+2), e.g.

10. 
    ajtowns commented at 3:06 am on March 26, 2022: member

    https://github.com/ajtowns/bitcoin/commits/202203-assume has a couple of commits implementing this approach if anyone wants to take a look
11. 
    ajtowns commented at 3:16 am on March 30, 2022: member

    PR’d as #24714
12. MarcoFalke referenced this in commit 87dc1dc55f on Mar 31, 2022
13. MarcoFalke closed this on Mar 31, 2022

    ---

14. sidhujag referenced this in commit 2cd931a679 on Apr 3, 2022
15. DrahtBot locked this on Mar 31, 2023

Contributors
ajtowns sipa MarcoFalke

Labels
Feature