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

theuni commented at 6:24 pm on June 26, 2020: member

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:

0Span<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});

in src/span.h:110 in 6d47b23f79 outdated

107+
108+    template <typename V>
109+    constexpr Span(const V& other SPAN_ATTR_LIFETIMEBOUND,
110+        typename std::enable_if<!is_Span<V>::value &&
111+                                std::is_convertible<typename std::remove_pointer<decltype(std::declval<const V&>().data())>::type (*)[], C (*)[]>::value &&
112+                                std::is_convertible<decltype(std::declval<V&>().size()), std::size_t>::value, std::nullptr_t>::type = nullptr)

sipa commented at 6:43 pm on June 26, 2020:

Nit: declval<const V&>. Not that I expect size() to differ between const and non-const objects…

theuni commented at 5:55 pm on June 29, 2020:

Fixed, thanks.

sipa commented at 10:07 pm on June 26, 2020: member

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 commented at 1:02 pm on June 28, 2020: contributor

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?

 0diff --git a/src/attributes.h b/src/attributes.h
 1index 45099bd8b..9d5c1d44a 100644
 2--- a/src/attributes.h
 3+++ b/src/attributes.h
 4@@ -19,4 +19,14 @@
 5 #  endif
 6 #endif
 7 
 8+#if defined(__clang__)
 9+#  if __has_attribute(lifetimebound)
10+#    define LIFETIMEBOUND [[clang::lifetimebound]]
11+#  else
12+#    define LIFETIMEBOUND
13+#  endif
14+#else
15+#  define LIFETIMEBOUND
16+#endif
17+
18 #endif // BITCOIN_ATTRIBUTES_H

jonatack commented at 1:29 pm on June 28, 2020: member

Concept ACK

theuni force-pushed on Jun 29, 2020

span: (almost) match std::span's constructor behavior

c++20's draft of std::span no longer includes move constructors.

62733fee87

theuni force-pushed on Jun 29, 2020

theuni commented at 5:57 pm on June 29, 2020: member

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 force-pushed on Jun 29, 2020

span: add lifetimebound attribute

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});

1d58cc7cb0

theuni force-pushed on Jun 29, 2020

theuni commented at 7:22 pm on June 29, 2020: member

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:

0$ 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 commented at 4:27 am on July 1, 2020: member

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

I like magic.

ACK 1d58cc7cb040a70f768b632f294db4e0797d3a34

MarcoFalke commented at 12:25 pm on July 1, 2020: member

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 1d58cc7cb040a70f768b632f294db4e0797d3a34

MarcoFalke added the label Refactoring on Jul 1, 2020

in src/span.h:23 in 1d58cc7cb0 outdated

17@@ -18,6 +18,16 @@
18 #define ASSERT_IF_DEBUG(x)
19 #endif
20 
21+#if defined(__clang__)
22+#if __has_attribute(lifetimebound)
23+#define SPAN_ATTR_LIFETIMEBOUND [[clang::lifetimebound]]

MarcoFalke commented at 8:14 pm on July 1, 2020:

0#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 commented at 6:25 pm on July 7, 2020:

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 commented at 7:55 pm on July 7, 2020:

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

sipa commented at 11:30 pm on July 1, 2020: member

@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 commented at 0:19 am on July 2, 2020: member

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 (#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 commented at 4:56 pm on July 3, 2020: member

Non-expert code review ACK 1d58cc7cb040a

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 commented at 10:48 pm on July 4, 2020: member

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

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

Add lifetimebound to attributes for general-purpose usage

Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>

e3e7446305

theuni force-pushed on Jul 8, 2020

sipa commented at 5:46 pm on July 8, 2020: member

ACK e3e7446305329ce96e9cf5f5161658eb2e1ea888

sipa closed this on Jul 8, 2020

sipa reopened this on Jul 8, 2020

sipa commented at 5:46 pm on July 8, 2020: member

Sorry, I misclicked.

in src/attributes.h:30 in e3e7446305

25+#  else
26+#    define LIFETIMEBOUND
27+#  endif
28+#else
29+#  define LIFETIMEBOUND
30+#endif

jonatack commented at 10:45 am on July 9, 2020:

Build warning (in clang 11.0, but not gcc 9.3)

0In file included from validation.cpp:6:
1In file included from ./validation.h:23:
2In file included from ./txdb.h:10:
3In file included from ./dbwrapper.h:12:
4In file included from ./util/system.h:17:
5./attributes.h:30:7: warning: extra tokens at end of #endif directive [-Wextra-tokens]
6#endif#if defined(__clang__)
7      ^
8      //
91 warning generated.

0((HEAD detached at origin/pr/19387))$ clang --version
1Debian clang version 11.0.0-++20200708111116+1f84ace3c72-1~exp1~20200708091733.3345 
2Target: x86_64-pc-linux-gnu

theuni commented at 4:44 pm on July 9, 2020:

I don’t understand what this is complaining about. Looks like some subtle whitespace thing, but it’s all fine as far as I can tell. Why wouldn’t it warn about the block above as well?

jonatack commented at 4:47 pm on July 9, 2020:

Same. Maybe that version of clang is buggy if no one else got it.

theuni commented at 4:59 pm on July 9, 2020:

I can’t hit this with clang 10.

Does it spew a million warnings for our includes? That would make sense, at least. But if this is the only one there must be a reason.

If this is a real clang bug, feel free to bisect and report upstream since you’re building from git, so we don’t hit this when 11 is released :)

jonatack commented at 5:38 pm on July 9, 2020:

Installed the new clang nightly updates to this version, built, and the warning is gone. (Yay!)

0$ clang --version
1Debian clang version 11.0.0-++20200709111134+dc4a6f5db4f-1~exp1~20200709091753.3347
2
3$ uname -a
4Linux 5.7.0-1-amd64 [#1](/bitcoin-bitcoin/1/) SMP Debian 5.7.6-1 (2020-06-24) x86_64 GNU/Linux

jonatack commented at 10:46 am on July 9, 2020: member

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

sipa commented at 7:34 pm on July 14, 2020: member

Mostly a reminder for myself: WithParams, ParamsStream, and ParamsWrapper in #19503 could also use LIFETIMEBOUND.

sipa commented at 9:04 pm on July 30, 2020: member

Anyone else feel like reviewing this?

JeremyRubin commented at 11:29 pm on July 30, 2020: contributor

I’m curious how this plays with c++17, if/when we switch to std::span will we be able to keep the annotation somehow?

sipa commented at 11:31 pm on July 30, 2020: member

@JeremyRubin We can keep annotations on our own functions, but at least MakeSpan won’t be needed anymore. On std::span we can’t put our own annotations of course, but presumably the stdlib that ships with c++20 supporting clang will have this annotation built-in for its span type.

(span is c++20, not c++17)

JeremyRubin commented at 11:32 pm on July 30, 2020: contributor

ok can revisit in 10 years ;)

ajtowns commented at 2:33 pm on September 22, 2020: member

ACK e3e7446305329ce96e9cf5f5161658eb2e1ea888 (drive by; only a quick skim of code and some basic sanity checks)

MarcoFalke commented at 2:18 pm on November 25, 2020: member

review ACK e3e7446305329ce96e9cf5f5161658eb2e1ea888 🔗

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK e3e7446305329ce96e9cf5f5161658eb2e1ea888 🔗
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUi3Mgv+Lgqd8NfB61HB1uzfd9S9SLQ0uPmWfMMNwBxuueD7YyuhvsNS2sto+2cK
 83dwf5m0EkQHymOEWtPZRAR1TFgr+phnM7ky4/MEJkECSiNWWHWCNqFybzpNvoPjt
 9lPAvduAM9OO4ZiHRPgqTsVGhz01hGpN7D+Hjo7bMu5y5yv5UFaOhz90earJ/44AI
10cWiZTw2YVmO9MZBWnBUUFUwj7OwBN5kn8SXHhe6/EoZkECpD/aIMXElUB5gmu+l+
11ko11vuJdzl4dQpf9v6oPl/0kSACwwf8nEpAM+LjuHOMcGZKrCmwPwmRF8tRmLAY5
12p/BwPR3wvA7EQaqJOVnZ+S1GcAEpFt5J8eQ3Vn0Z0qyG+7LQ6om3TSkWEP2fqZQ7
13h0L/ZJwbSd5nuvVV4D3du+P0SoKW6nAN1W8Iz+Yrt45CnoZ1n2v3dVLUw0S/HyIF
14BIAVD9e2T6wznmOsOsZJpX38l4fORnwXpY7jH2mO7nFJmOQ8r8dRiVyR/obkk/Sq
15psWzYP6c
16=x28h
17-----END PGP SIGNATURE-----

Timestamp of file with hash c85bdf628bd486c6b0345f5d023467302549bb7e57ac1eb39166395d465fbb11 -

MarcoFalke merged this on Nov 25, 2020

MarcoFalke closed this on Nov 25, 2020

sidhujag referenced this in commit f2c6cfacd9 on Nov 25, 2020

kittywhiskers referenced this in commit 04dff9a02d on Mar 10, 2021

kittywhiskers referenced this in commit 278a71c8f9 on Mar 10, 2021

kittywhiskers referenced this in commit 6adf24a57b on Mar 10, 2021

kittywhiskers referenced this in commit 5f0720261f on Mar 10, 2021

kittywhiskers referenced this in commit f2d62e7ed8 on Mar 10, 2021

kittywhiskers referenced this in commit dc62d4d300 on Mar 12, 2021

kittywhiskers referenced this in commit e8782af572 on Mar 19, 2021

kittywhiskers referenced this in commit 7feadd8fc1 on Mar 23, 2021

kittywhiskers referenced this in commit be1929d92d on Mar 23, 2021

kittywhiskers referenced this in commit 17f15e4170 on Mar 23, 2021

kittywhiskers referenced this in commit 574cfa0916 on Mar 23, 2021

kittywhiskers referenced this in commit 51a0633604 on Mar 23, 2021

kittywhiskers referenced this in commit 1be361a16e on Mar 23, 2021

kittywhiskers referenced this in commit 127f847ce9 on Mar 23, 2021

kittywhiskers referenced this in commit 2a643a320a on Mar 23, 2021

kittywhiskers referenced this in commit 768ee4e397 on Mar 23, 2021

kittywhiskers referenced this in commit 1e79621943 on Apr 18, 2021

kittywhiskers referenced this in commit 5667698546 on Apr 23, 2021

UdjinM6 referenced this in commit bf01ed7881 on Apr 27, 2021

random-zebra referenced this in commit 56f10aaa51 on Jul 21, 2021

random-zebra referenced this in commit b4751e10ce on Aug 11, 2021

Fabcien referenced this in commit 1349291f14 on Jan 26, 2022

DrahtBot locked this on Feb 15, 2022

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