Add LIFETIMEBOUND to CScript where needed #22278

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:

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

Before: (no warning) After:

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

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.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

@theuni want to take a look here?

utACK

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?

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:

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

utACK fa7e6c56f58678b310898a158053ee9ff8b27fe7.

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