span: Add lifetimebound attribute to guard against temporary lifetime issues #19382
Conversation
These have the effect of disabling the default move constructor and move
operator. This is generally meaningless for spans due to their simplicity,
but consider the (simplified here) internal implementation of .first():
Span<C> first(std::size_t count) const noexcept {
return Span<C>(m_data, count);
}
Technically this creates a new Span object and moves/copies it when it returns,
but RVO takes care of optimizing that out.
Because trivial moves have been implicitly deleted, it falls into the more
complicated conversion rvalue constructor to handle the return.
The next commit will add an attribute to the conversion constructor to help
detect dangling stack references, so we want to make sure that we're not
sending trivial copies through it.
NOTE: This copy/move is could also be avoided by constructing the return value
in-place:
Span<C> first(std::size_t count) const noexcept {
return {m_data, count};
}
See http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0936r0.pdf for reference. This helps to guard against dangling references caused by construction from temporaries such as: Span<const int> sp(std::vector<int>{1,2,3});
| @@ -18,6 +18,12 @@ | |||
| #define ASSERT_IF_DEBUG(x) | |||
| #endif | |||
|
|
|||
| #if defined(__clang__) && __has_attribute(lifetimebound) | |||
There was a problem hiding this comment.
This seems to fail on gcc 4.8:
libtool: compile: /usr/bin/ccache g++ -m32 -std=c++11 -DHAVE_CONFIG_H -I. -I../src/config -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I./obj -I./secp256k1/include -DBUILD_BITCOIN_INTERNAL -I/home/travis/build/bitcoin/bitcoin/depends/i686-pc-linux-gnu/share/../include/ -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -D_FILE_OFFSET_BITS=64 -fstack-reuse=none -Wstack-protector -fstack-protector-all -pipe -O2 -fno-extended-identifiers -fvisibility=hidden -c consensus/merkle.cpp -fPIC -DPIC -o consensus/.libs/libbitcoinconsensus_la-merkle.o
In file included from ./serialize.h:25:0,
from ./script/script.h:11,
from ./primitives/transaction.h:11,
from ./primitives/block.h:9,
from ./consensus/merkle.h:10,
from consensus/merkle.cpp:5:
./span.h:21:42: error: missing binary operator before token "("
#if defined(__clang__) && __has_attribute(lifetimebound)
^
There was a problem hiding this comment.
Thanks, will break this up into nested if's.
| constexpr Span(const Span&) noexcept = default; | ||
|
|
||
| /** Default assignment operator. */ | ||
| Span& operator=(const Span& other) noexcept = default; |
There was a problem hiding this comment.
in commit 397e5ed
It looks like std::span has no move constructor (https://en.cppreference.com/w/cpp/container/span/span), so shouldn't we do the same?
There was a problem hiding this comment.
Hmm. I'm seeing conflicting info on this.
According to the (official?) revision 7 draft, the move constructor was removed in r6. See here: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0122r7.pdf
If we were to follow that, I believe we would drop our fun rvalue ctor.
I'm not sure what draft version cppreference is referencing, but I believe it must be an old one.
libc++ for example has implemented r7: https://reviews.llvm.org/D49338.
Will explore more.
There was a problem hiding this comment.
libc++ for example has implemented r7: https://reviews.llvm.org/D49338.
looking at the source code, it shows they are using = default, no?
There was a problem hiding this comment.
@MarcoFalke Yes, but they also don't have a universal reference constructor like we do.
The problem is that return values are passed through a move constructor if they exist, before copy constructors. See here under "Automatic move from local variables and parameters".
Because the default move constructor is removed by adding the default constructor/assignment operator, our return values are passed through our universal constructor as rvalues. That's really not a problem except that we want to minimize what's going through that constructor so that we can apply the lifetimebound attribute to it.
( Bonus TIL: apparently c++17 does away with this return value path altogether when possible: "If expression is a prvalue, the result object is initialized directly by that expression. This does not involve a copy or move constructor when the types match" )
So there are 2 possible solutions for us (if you're willing to accept that there's something to be solved)
- remove the default ctor/assignment operator as I've done here, thus enabling a trivial move constructor for return values to pass through
- remove our universal reference constructor and replace it with something less greedy
Your question made me look up the current c++20 draft spec, where I noticed that the universal reference constructor has been removed in a newish revision. So what's on cppreference is actually stale.
But that gives us an easy answer to the question above, we should just update our implementation to match the latest draft spec. I'm going to close this and open a new PR with that approach, since it's sufficiently different from what's here.
Since clang has already has it implemented, presumably libc++ will ship with this attribute (or keyword) wired up once the standard is finished. |
|
Concept ACK. |
… add lifetimebound attribute e3e7446 Add lifetimebound to attributes for general-purpose usage (Cory Fields) 1d58cc7 span: add lifetimebound attribute (Cory Fields) 62733fe span: (almost) match std::span's constructor behavior (Cory Fields) Pull request description: Replaces #19382 with a different approach. See [this comment](#19382 (comment)) for the reasoning behind the switch. -- Description from #19382: See [here](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0936r0.pdf) for more detail on lifetimebound. This is implemented using preprocesor macros rather than configure checks in order to keep span.h self-contained. The ```[[clang::lifetimebound]]``` syntax was chosen over ```__attribute__((lifetimebound))``` because the former is more flexible and works to guard ```this``` as well as function parameters, and also because at least for now, it's available only in clang. There are currently no violations in our codebase, but this can easily be tested by inserting one like this somewhere and compiling with a modern clang: ```c++ Span<const int> bad(std::vector<int>{1,2,3}); ``` The result: > warning: temporary whose address is used as value of local variable 'bad' will be destroyed at the end of the full-expression [-Wdangling] Span<const int> bad(std::vector<int>{1,2,3}); ``` ACKs for top commit: sipa: ACK e3e7446 ajtowns: ACK e3e7446 (drive by; only a quick skim of code and some basic sanity checks) MarcoFalke: review ACK e3e7446 🔗 jonatack: ACK e3e7446 change since last review is adding `[[clang::lifetimebound]]` as `LIFETIMEBOUND` to src/attributes.h as suggested in #19387 (comment). Tree-SHA512: 05a3440ee595ef0e8d693a2820b360707695c016a68e15df47c20cd8d053646cc6c8cca8addd7db40e72b3fce208879a41c8102ba7ae9223e4366e5de1175211
See here for more detail on lifetimebound.
This is implemented using preprocesor macros rather than configure checks in order to keep span.h self-contained.
The
[[clang::lifetimebound]]syntax was chosen over__attribute__((lifetimebound))because the former is more flexible and works to guardthisas well as function parameters, and also because at least for now, it's available only in clang.There are currently no violations in our codebase, but this can easily be tested by inserting one like this somewhere and compiling with a modern clang:
The result: