"Lifetime extension" issue

[Quuxplusone]

Hi folks, I'm the author of the -Wreturn-std-move diagnostic that triggered issue #847.

This issue was marked "fixed" after commit ded84836, which changed the code from

CryptoBuffer&& key = GenerateXRandomBytes(keyLengthBytes, false);
// ...
return key;  // warning: no copy elision, no implicit move, key is (surprisingly) always copied

to

const CryptoBuffer& key = GenerateXRandomBytes(keyLengthBytes, false);
// ...
return key;  // key is (obviously) always copied

The compiler's suggested fix had been:

CryptoBuffer&& key = GenerateXRandomBytes(keyLengthBytes, false);
// ...
return std::move(key);  // no copy elision, but key is implicitly moved (not copied)

However, it appears to me that an even better fix would have been:

CryptoBuffer key = GenerateXRandomBytes(keyLengthBytes, false);
// ...
return key;  // copy elision occurs

Could someone (@wps132230, @marcomagdy, @Bu11etmagnet) explain the rationale for wanting to copy the CryptoBuffer in this case? I think the part I'm having the most trouble understanding is why you want to use lifetime-extended temporaries instead of simple (non-reference) automatic variables in the first place. What's the advantage to this codebase of using lifetime-extension?

Also, technical sidebar: lifetime-extension works just fine on rvalue references. The only thing "wrong" with the original code was the thing Clang was telling you about: that the statement return key; looks like it should trigger implicit move, but in fact it doesn't. (And the reason it doesn't is that key is a reference, not an automatic variable. That's why I say that on the surface it seems that just making it a variable would be the obvious "better" fix.)

[Bu11etmagnet]

I certainly don't want to copy the CryptoBuffer. However, people tend to over-use rvalue references when they want to lifetime-extend their temporaries (especially if they haven't been diligent with marking their methods as const). Which is why my first thought was to replace the rvalue reference with a const reference.

With modern C++ compilers (especially C++17 conforming, where copy elision is mandatory), your suggestion (return by value) is likely the best solution. We'll have copy elision when returning from this function (GenerateKey), and hopefully also when the CryptoBuffer is returned from GenerateXRandomBytes. Even if not all copies are elided, if CryptoBuffer is move-aware, then at worst we get a few move constructions here and there.

[Quuxplusone]

You'll have copy elision in both places, yes; that's been true on all mainstream compilers (GCC/Clang/EDG/MSVC/Intel) since circa 2006.

I was confused because in #847 the compiler directly told you "Hey, you're making a copy when a move would be cheaper," and your reaction was not "ok cool let's add a std::move like it told us to" but instead to double down on the copy (like, to the point of const-qualifying the source operand). So I thought there might be some rationale there that I was missing.

[Bu11etmagnet]

Note that whether we'll have copy elision when returning from GenerateXRandomBytes depends on the implementation of GenerateXRandomBytes . You can't tell just by looking at the caller (the code that was changed ded8483 ).

[marcomagdy]

lifetime-extension works just fine on rvalue references.

This was the missing piece on my part. Thanks for bringing it up @Quuxplusone.

Because we still support older compilers, I can't guarantee copy-elision. However, once we move the SDK to C++17 I will switch this to return by value. For now, I will move it back to the rvalue return by std::move-ing.

That being said, I can't reproduce the warning anymore (with the old code) I've tried with Clang 6 ~or~ and GCC 7.3. @Bu11etmagnet what are your CMake parameters?

[Quuxplusone]

I can't reproduce the warning anymore [...] with Clang 6

The diagnostic -Wreturn-std-move has been in Clang trunk only since February 2018; I don't think it would have been in time for Clang 6.

[Bu11etmagnet]

cmake -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang -DCPP_STANDARD=11  \
-DCMAKE_BUILD_TYPE=Debug \
-DCUSTOM_MEMORY_MANAGEMENT=0 -DBUILD_ONLY="lib1;lib2" \
../aws-sdk-cpp

clang++ is clang trunk, built daily from source.

[marcomagdy]

In https://github.com/aws/aws-sdk-cpp/issues/847 you said you are using gcc version 7.2.0 (Ubuntu 7.2.0-8ubuntu3.2). Was that a mistake?

[Quuxplusone]

@marcomagdy: I can 100% confirm that -Wreturn-std-move has never appeared in any version of GCC as of this writing. (But people do sometimes build with multiple compilers and get confused between them...)

[Bu11etmagnet]

I have more than one compiler and I'm not afraid to use them! :)

[github-actions[bot]]

Greetings! Sorry to say but this is a very old issue that is probably not getting as much attention as it deservers. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to open a new one.

[Quuxplusone]

Indeed, the offending code is still there at https://github.com/aws/aws-sdk-cpp/blob/9e0c8b0831e89c5b562042bf5d0315551ca201ad/aws-cpp-sdk-core/source/utils/crypto/Cipher.cpp#L112

The main issue here is that you're suppressing NRVO (a.k.a. copy elision), which degrades performance for no benefit.

The secondary issue is that unnecessarily relying on lifetime extension can degrade the ability of maintainers to understand the code. See https://quuxplusone.github.io/blog/tags/#lifetime-extension

[github-actions[bot]]

Greetings! Sorry to say but this is a very old issue that is probably not getting as much attention as it deservers. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to open a new one.

[Quuxplusone]

Confirmed, this is still a problem. https://github.com/aws/aws-sdk-cpp/blob/e009d9a/aws-cpp-sdk-core/source/utils/crypto/Cipher.cpp#L112

@marcomagdy: Please make the fix you talked about in https://github.com/aws/aws-sdk-cpp/issues/920#issuecomment-409013155 . It's been three years; you should just do it.

I'm going to attempt to unsubscribe from this issue now.

[wps132230]

Hi @Quuxplusone, Marco is not in the team anymore, I will take care of this issue.

[github-actions[bot]]

This issue is now closed. Comments on closed issues are hard for our team to see. If you need more assistance, please open a new issue that references this one.