doc: Span pitfalls

sipa commented at 2:08 am on June 24, 2020: member

This is an attempt to document pitfalls with the use of Span, following up on comments like #18468 (comment) and #18468 (review)

sipa commented at 2:08 am on June 24, 2020: member

Ping @theuni.

fanquake added the label Docs on Jun 24, 2020

laanwj commented at 12:50 pm on June 24, 2020: member

ACK 402980d7d0299769e147688250235339908523c7 It might aid discoverability to refer to this list of pitfalls in developer-notes.md.

jb55 commented at 2:27 pm on June 24, 2020: member

ACK 402980d7d0299769e147688250235339908523c7

practicalswift commented at 2:55 pm on June 24, 2020: contributor

ACK modulo adding a reference in developer-notes.md to ease discoverability as suggested by @laanwj :)

Prediction: We’ll need a similar life-time issue warning in the developer notes once we get the sharp edges of std::string_view to play with :)

in src/span.h:51 in 402980d7d0 outdated

46+ *   The lifetime of the vector ends when the statement it is created in ends.
47+ *   Thus the Span is left with a dangling reference, and using it is undefined.
48+ *
49+ * - Due to Span's automatic creation from range-like objects (arrays, and data
50+ *   types that expose a data() and size() member function), functions that
51+ *   accept as Span as input parameter can be called with any compatible

jnewbery commented at 3:01 pm on June 24, 2020:

s/accept as Span/accept a Span/

sipa commented at 8:51 pm on June 26, 2020:

Fixed.

in src/span.h:44 in 402980d7d0 outdated

39+ * - One particular pitfall is that Spans can be constructed from temporaries,
40+ *   but this is unsafe when the Span is stored in a variable, outliving the
41+ *   temporary. For example, this will compile, but exhibits undefined behavior:
42+ *
43+ *       Span<const int> sp(std::vector<int>{1, 2, 3});
44+ *       printf("%i\n", sp.front()); // UB!

theuni commented at 4:36 pm on June 24, 2020:

Turns out clang actually has a newish attribute to detect this! https://reviews.llvm.org/rL338464

I can confirm that this case is detected with clang-10 and the following change to span.h:

0-    constexpr Span(V&& v) noexcept : m_data(v.data()), m_size(v.size()) {}
1+    constexpr Span(V&& v __attribute__((lifetimebound))) noexcept : m_data(v.data()), m_size(v.size()) {}

A new warning is given:

0spantest.cpp:12:24: warning: temporary whose address is used as value of local variable 'sp' will be destroyed at the end of the full-expression [-Wdangling]
1    Span<const int> sp(std::vector<int>{1, 2, 3});
2                       ^~~~~~~~~~~~~~~~~~~~~~~~~
31 warning generated.

It currently warns about several uses in #13062. I’m going through them now to see if they’re false-positives.

sipa commented at 4:47 pm on June 24, 2020:

@theuni Interesting!

I expect that there is one particular false positive: when you’re converting a temporary Span to a compatible one. We could introduce a separate constructor for that, which then doesn’t get the attribute. On the other hand, perhaps that interferes with useful detection too.

theuni commented at 5:53 pm on June 24, 2020:

@sipa Yes, that seems to be the bulk of them.

~~I can’t figure out why this isn’t UB (from ParseScript()):~~

0    if (Func("pkh", expr)) {
1        auto pubkey = ParsePubkey(key_exp_index, expr, ctx != ParseScriptContext::P2WSH, out, error);

Remember that Func() takes a std::string, not a c string. So in the example above, presumably expr ends up pointing to somewhere inside the temporary string’s memory after Func() returns. As you pointed out, for string literals that’s the lifetime of the program. But in this case, I think we’re relying on the fact that std::string’s .data() returns that exact pointer. Is that guaranteed to be the case?

Edit: Nevermind. Guess I read the body of Func() too quickly. The modified span is not created from the string param.

theuni commented at 6:58 pm on June 24, 2020:

We could introduce a separate constructor for that, which then doesn’t get the attribute. On the other hand, perhaps that interferes with useful detection too.

Indeed giving Span a (default) move ctor causes all current lifetimebound false-positives to go away, but still warns when intentionally inserting your UB example.

sipa commented at 8:51 pm on June 26, 2020:

Discussion on this moved to #19387.

in src/span.h:58 in 402980d7d0 outdated

53+*
54+ *       void Foo(Span<const int> arg);
55+ *
56+ *       Foo(std::vector<int>{1, 2, 3}); // Works
57+ *
58+ *   This is very useful in cases a function truly does not care about the

amitiuttarwar commented at 4:43 pm on June 24, 2020:

in cases where a function..

sipa commented at 8:51 pm on June 26, 2020:

Fixed.

in src/span.h:71 in 402980d7d0 outdated

66+ *
67+ *       FooMut(std::vector<int>{1, 2, 3}); // Does not compile
68+ *       std::vector<int> baz{1, 2, 3};
69+ *       FooMut(baz); // Works
70+ *
71+ *   This is similar to how functions that take (non-const) lvalue reference

ysangkok commented at 3:39 pm on June 25, 2020:

reference should be plural or have article

sipa commented at 8:50 pm on June 26, 2020:

Done.

theuni commented at 10:01 pm on June 25, 2020: member

Related: I’ve pushed a branch here which adds the lifetimebound annotation: https://github.com/theuni/bitcoin/commits/lifetimebound

The changes are actually very simple, but the commit messages are verbose and reflect my (hopefully correct?) understanding of what’s going on. @sipa Would you like me to PR that separately for discussion? Or (if you actually want them) do you want to pull them into #13062 ?

sipa commented at 10:10 pm on June 25, 2020: member

@theuni That looks great; I think you should PR it separately (as master stands to benefit from it independent of any other Span-related changes).

theuni approved

theuni commented at 10:28 pm on June 25, 2020: member

ACK 402980d7d0299769e147688250235339908523c7 modulo the nits that have been pointed out.

MarcoFalke renamed this:
~~[comments only] Document Span pitfalls~~
doc: Span pitfalls
on Jun 25, 2020

MarcoFalke commented at 11:56 pm on June 25, 2020: member

ACK 402980d7d0299769e147688250235339908523c7

doc: Document Span pitfalls 3502a60418

sipa force-pushed on Jun 26, 2020

sipa commented at 8:50 pm on June 26, 2020: member

Addressed all comments, I believe.

doc: Mention Span in developer-notes.md fab57e2b9b

sipa force-pushed on Jun 26, 2020

laanwj commented at 1:18 pm on June 29, 2020: member

ACK fab57e2b9bc4577fcfcd9fbddbc35d96046c5d88

laanwj merged this on Jun 29, 2020

laanwj closed this on Jun 29, 2020

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 13597e2d22 on Aug 30, 2021

DrahtBot locked this on Feb 15, 2022

doc: Span pitfalls #19367