span: update constructors to match c++20 draft spec and add lifetimebound attribute

[theuni]

Replaces #19382 with a different approach. See this comment for the reasoning behind the switch.

--

Description from #19382:

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 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:

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 bad(std::vector{1,2,3});

[sipa]

ACK apart from nit above.

I've tested this by rebasing #13062 and #19326 on top, and compiling with Clang 10; no warnings. Adding an obvious Span<const int> bad{std::vector<int>{}}; does produce a warning.

It does seem that no warning is produced when MakeSpan is used in between, and I can't get that fixed. I think that's fine, but it'd be nice to improve upon.

[practicalswift]

Concept ACK: [[clang::lifetimebound]] is great! :)

I think this will be useful also outside of src/span.{cpp,h}. What about adding it to src/attributes.h as LIFETIMEBOUND?

diff --git a/src/attributes.h b/src/attributes.h
index 45099bd8b..9d5c1d44a 100644
--- a/src/attributes.h
+++ b/src/attributes.h
@@ -19,4 +19,14 @@
 #  endif
 #endif
 
+#if defined(__clang__)
+#  if __has_attribute(lifetimebound)
+#    define LIFETIMEBOUND [[clang::lifetimebound]]
+#  else
+#    define LIFETIMEBOUND
+#  endif
+#else
+#  define LIFETIMEBOUND
+#endif
+
 #endif // BITCOIN_ATTRIBUTES_H

[jonatack]

Concept ACK

[theuni]

Updated and squashed.

@practicalswift I'd rather wait to add it there until there's a use for it, if that's ok. By the time we have it to use somewhere else, it may not be clang-only anymore.

I did change it to use __has_cpp_attribute to match our existing attribute check, though.

Edit: whoops, __has_cpp_attribute doesn't actually work here! Back to __has_attribute.

[theuni]

Sorry for the pushing/rebasing noise, done now.

It does seem that no warning is produced when MakeSpan is used in between, and I can't get that fixed. I think that's fine, but it'd be nice to improve upon.

@sipa It turns out that this warning does work for MakeSpan, but only in c++17 mode and above:

$ clang++ spantest.cpp -o spantest -std=c++17

spantest.cpp:10:26: warning: temporary whose address is used as value of local variable 'temp' will be destroyed at the end of the full-expression [-Wdangling]
auto temp = MakeSpan(std::vector{1,2,3});

I've gone ahead and added the annotations there as well, so they should just magically work when we switch to c++17.

[sipa]

so they should just magically work when we switch to c++17.

I like magic.

ACK 1d58cc7

[maflcko]

Instead of magic, I'd rather remove them and add the corresponding constructors from c++20 when we switched to c++17

second-commit-only review ACK 1d58cc7

[maflcko]

Suggested change

	#define SPAN_ATTR_LIFETIMEBOUND [[clang::lifetimebound]]
	#define ATTR_LIFETIMEBOUND [[clang::lifetimebound]]

Can this be named a bit more generic, so that it can be used in other places such as #19426 ?

[theuni]

I namespaced it to avoid colliding with a more general define. As span.h is designed to be an abstraction, I'd really prefer to keep it an island and not require that it include another header.

Since @sipa was quick to point out a few places where this could already be useful, how about going with @practicalswift's idea of adding a second, more generic define in attributes.h ?

[sipa]

@theuni Not sure how to weigh code duplication vs. a dependency on attributes.h. I'm fine with either.

[sipa]

@MarcoFalke Well, no reason why we can't have both. The attribute as argument to MakeSpan makes sense - even if the compiler doesn't do anything useful with it - as it documents behavior to the user as well. And perhaps clang 123091137.4 will support it even in C++11 mode - who knows.

[sipa]

There are a lot more cases in the codebase where a data type stores a reference or pointer to another object provided to the constructor exist. For example:

• In src/stream.h: OverrideStream, CVectorReader, BitStreamReader, BitStreamWriter
• In src/script/sign.h: MutableTransactionSignatureCreator (refactor: Change * to & in MutableTransactionSignatureCreator #19426).
• In src/script/interpreter.h: GenericTransactionSignatureChecker
• In src/serialize.h: Wrapper, Using
• In src/wallet/rpcwallet.cpp: DescribeWalletAddressVisitor
• In src/key_io.cpp: DestinationEncoder
• In src/dbwrapper.h: CDBBatch, CDBIterator
• In src/flatfile.h: FlatFileSeq
• In src/hash.h: CHashVerifier
• In src/miner.h: CBlockAssembler
• In src/net_processing.h: PeerLogicValidation
• In src/scheduler.h: SingleThreadedSchedulerClient
• In src/sync.h: CSemaphoreGrant
• In src/validation.h: CScriptCheck, ChainstateManager, CChainState

Several of these accept const lvalue references, and store them, and are at risk already. Others take in a mutable lvalue reference or pointers, but would make sense to support rvalue/universal references as input (so that temporaries can be passed to it). In all those cases, having a lifetimebound attribute would be useful.

So I think that means we should just put it in attributes.h. There is plenty of potential for it.

[jonatack]

Non-expert code review ACK 1d58cc7

Looked at the linked doc and other docs/implementations I could find, abseil/span.h etc. This compiled fine including with it added to src/attributes.h. Unfortunately I am currently stuck on Debian Clang 6 which is less helpful.

[jonatack]

Upgraded to Clang 11 and now see the -Wdangling warning mentioned in the PR description.

wallet/rpcwallet.cpp:42:22: warning: temporary whose address is used as value of local variable
'a_bad_vec' will be destroyed at the end of the full-expression [-Wdangling]
Span<const int> a_bad_vec(std::vector<int>{1,2,3});
                     ^~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

[sipa]

ACK e3e7446

[sipa]

Sorry, I misclicked.

[jonatack]

ACK e3e7446 change since last review is adding [[clang::lifetimebound]] as LIFETIMEBOUND to src/attributes.h as suggested in #19387 (comment).