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

pull theuni wants to merge 3 commits into bitcoin:master from theuni:lifetimebound2 changing 2 files +43 −4
  1. 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});

  2. 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.
  3. 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.

  4. 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
    
  5. jonatack commented at 1:29 pm on June 28, 2020: member
    Concept ACK
  6. theuni force-pushed on Jun 29, 2020
  7. span: (almost) match std::span's constructor behavior
    c++20's draft of std::span no longer includes move constructors.
    62733fee87
  8. theuni force-pushed on Jun 29, 2020
  9. theuni force-pushed on Jun 29, 2020
  10. 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.

  11. theuni force-pushed on Jun 29, 2020
  12. 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
  13. theuni force-pushed on Jun 29, 2020
  14. 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.

  15. 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

  16. 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

  17. MarcoFalke added the label Refactoring on Jul 1, 2020
  18. 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.
  19. 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.
  20. 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.

  21. 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.

  22. 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.
    
  23. Add lifetimebound to attributes for general-purpose usage
    Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
    e3e7446305
  24. theuni force-pushed on Jul 8, 2020
  25. sipa commented at 5:46 pm on July 8, 2020: member
    ACK e3e7446305329ce96e9cf5f5161658eb2e1ea888
  26. sipa closed this on Jul 8, 2020

  27. sipa reopened this on Jul 8, 2020

  28. sipa commented at 5:46 pm on July 8, 2020: member
    Sorry, I misclicked.
  29. 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
    
  30. 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).
  31. 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.
  32. sipa commented at 9:04 pm on July 30, 2020: member
    Anyone else feel like reviewing this?
  33. 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?
  34. 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)

  35. JeremyRubin commented at 11:32 pm on July 30, 2020: contributor
    ok can revisit in 10 years ;)
  36. 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)
  37. 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 -

  38. MarcoFalke merged this on Nov 25, 2020
  39. MarcoFalke closed this on Nov 25, 2020

  40. sidhujag referenced this in commit f2c6cfacd9 on Nov 25, 2020
  41. kittywhiskers referenced this in commit 04dff9a02d on Mar 10, 2021
  42. kittywhiskers referenced this in commit 278a71c8f9 on Mar 10, 2021
  43. kittywhiskers referenced this in commit 6adf24a57b on Mar 10, 2021
  44. kittywhiskers referenced this in commit 5f0720261f on Mar 10, 2021
  45. kittywhiskers referenced this in commit f2d62e7ed8 on Mar 10, 2021
  46. kittywhiskers referenced this in commit dc62d4d300 on Mar 12, 2021
  47. kittywhiskers referenced this in commit e8782af572 on Mar 19, 2021
  48. kittywhiskers referenced this in commit 7feadd8fc1 on Mar 23, 2021
  49. kittywhiskers referenced this in commit be1929d92d on Mar 23, 2021
  50. kittywhiskers referenced this in commit 17f15e4170 on Mar 23, 2021
  51. kittywhiskers referenced this in commit 574cfa0916 on Mar 23, 2021
  52. kittywhiskers referenced this in commit 51a0633604 on Mar 23, 2021
  53. kittywhiskers referenced this in commit 1be361a16e on Mar 23, 2021
  54. kittywhiskers referenced this in commit 127f847ce9 on Mar 23, 2021
  55. kittywhiskers referenced this in commit 2a643a320a on Mar 23, 2021
  56. kittywhiskers referenced this in commit 768ee4e397 on Mar 23, 2021
  57. kittywhiskers referenced this in commit 1e79621943 on Apr 18, 2021
  58. kittywhiskers referenced this in commit 5667698546 on Apr 23, 2021
  59. UdjinM6 referenced this in commit bf01ed7881 on Apr 27, 2021
  60. random-zebra referenced this in commit 56f10aaa51 on Jul 21, 2021
  61. random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
  62. Fabcien referenced this in commit 1349291f14 on Jan 26, 2022
  63. DrahtBot locked this on Feb 15, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-11 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me