Add LIFETIMEBOUND to CScript where needed #22278

1. 
   MarcoFalke commented at 8:35 pm on June 18, 2021: member

   Without this, stack-use-after-scope can only be detected at runtime with ASan or code review, both of which are expensive.

   Use LIFETIMEBOUND to turn this error into a compile warning.

   See https://releases.llvm.org/12.0.0/tools/clang/docs/AttributeReference.html#lifetimebound

   Example:

   0const CScript a{WITH_LOCK(::cs_main, return CScript{} << OP_0 << OP_1)};
   

   Before: (no warning) After:

   0warning: returning reference to local temporary object [-Wreturn-stack-address]
   1    const CScript a{WITH_LOCK(::cs_main, return CScript{} << OP_0 << OP_1)};
   2                                                ^~~~~~~~~
   3./sync.h:276:65: note: expanded from macro 'WITH_LOCK'
   4#define WITH_LOCK(cs, code) [&]() -> decltype(auto) { LOCK(cs); code; }()
   5                                                                ^~~~
   

2. MarcoFalke added the label Refactoring on Jun 18, 2021
3. 
   sipa commented at 8:39 pm on June 18, 2021: member

   Interesting, I wouldn’t have considered using lifetimebound for those, but it makes sense.

   utACK fabcaf39cd1516923c818b382ee3478b340f3db7.

   My understanding is that this cannot introduce bugs if it compiles with issues, as the attribute only adds warning/errors.

4. 
   DrahtBot commented at 11:36 pm on June 18, 2021: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   No conflicts as of last run.

5. fanquake deleted a comment on Jun 21, 2021
6. 
   fanquake commented at 3:34 am on September 2, 2021: member

   @theuni want to take a look here?
7. 
   sipa commented at 3:43 am on September 2, 2021: member

   utACK
8. 
   theuni commented at 6:24 pm on September 2, 2021: member

   Concept ACK/utACK. I guess ideally we’d annotate anywhere we return references to *this or *this.foo?

   I tried switching the return type from decltype(auto) to auto, which works around this problem by allowing copies to be made as a typical return would, but that disables the ability to bind to a legit returned reference. However, it does have the nice side-effect of showing where these annotations would be useful :)

   The one it turned up is ChainstateManager::InitializeChainstate, maybe add an annotation there as well?

9. 
   MarcoFalke commented at 11:41 am on September 3, 2021: member

   Concept ACK/utACK. I guess ideally we’d annotate anywhere we return references to *this or *this.foo?

   Correct. I think where foo is a (trivial?) built-in type, the annotation isn’t needed because at least clang will figure it out by itself. Though, it shouldn’t hurt, so maybe someone writes a clang-tidy script to do the refactor in a follow-up?

   The one it turned up is ChainstateManager::InitializeChainstate, maybe add an annotation there as well?

   Thanks, added a commit. Can be tested with this snippet:

   0warning: returning reference to local temporary object [-Wreturn-stack-address]
   1    CChainState& c1 = WITH_LOCK(::cs_main, return ChainstateManager{}.InitializeChainstate(&mempool));
   2                                                  ^~~~~~~~~~~~~~~~~~~
   3./sync.h:276:65: note: expanded from macro 'WITH_LOCK'
   4#define WITH_LOCK(cs, code) [&]() -> decltype(auto) { LOCK(cs); code; }()
   5                                                                ^~~~
   61 warning generated.
   

10. Add LIFETIMEBOUND to CScript where needed fa5c896724
11. Add LIFETIMEBOUND to InitializeChainstate fa7e6c56f5
12. MarcoFalke force-pushed on Sep 3, 2021
13. 
    theuni commented at 4:35 pm on September 3, 2021: member

    utACK fa7e6c56f58678b310898a158053ee9ff8b27fe7.
14. 
    jonatack commented at 6:13 pm on September 3, 2021: member

    Light ACK fa7e6c56f58678b310898a158053ee9ff8b27fe7 debug build with clang 13, reproduced the example compiler warning in the pull description, and briefly looked at clang::lifetimebound support in earlier versions of clang; it is in clang 7 (https://releases.llvm.org/7.0.0/tools/clang/docs/AttributeReference.html#lifetimebound-clang-lifetimebound), did not see references to it in earlier docs
15. fanquake merged this on Sep 4, 2021
16. fanquake closed this on Sep 4, 2021

    ---

17. sidhujag referenced this in commit 22ad63e600 on Sep 4, 2021
18. MarcoFalke deleted the branch on Sep 5, 2021
19. MarcoFalke referenced this in commit d17bbc3c48 on May 4, 2022
20. sidhujag referenced this in commit cedc09f1e9 on May 4, 2022
21. DrahtBot locked this on Sep 5, 2022

Contributors
MarcoFalke sipa DrahtBot fanquake theuni jonatack

Labels
Refactoring