Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 40 additions & 42 deletions _posts/2018-04-20-totw-120.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,14 @@ order: "120"

Originally posted as TotW #120 on August 16, 2012

*by Samuel Benzaquen, [(sbenza@google.com)](mailto:sbenza@gmail.com)*
*by Samuel Benzaquen, [(sbenza@google.com)](mailto:sbenza@google.com)*

Let's suppose you have a code snippet like below, which relies on an RAII
cleanup function, which seems to be working as expected:
Let's suppose you have this code snippet, which seems to be working as expected:

```c++
MyStatus DoSomething() {
MyStatus status;
auto log_on_error = RunWhenOutOfScope([&status] {
absl::Status DoSomething() {
absl::Status status;
auto log_on_error = absl::MakeCleanup([&status] {
if (!status.ok()) LOG(ERROR) << status;
});
status = DoA();
Expand All @@ -31,45 +30,43 @@ MyStatus DoSomething() {
}
```

A refactor changes the last line from `return status;` to
`return MyStatus();` and suddenly the code stopped logging errors.
A refactor changes the last line from `return status;` to `return
absl::OkStatus();` and suddenly the code stopped logging the errors.

What is going on?

## Summary

Never access (read or write) the return variable of a function after the return
statement has run. Unless you take great care to do it correctly, the behavior
is unspecified.
Never access (read or write) the return variable after the return statement has
run. Unless you take great care to do it correctly, the behavior is unspecified.

Return variables are implicitly accessed by destructors after they have been
copied or move (See Section 6.6.3 of the C++11 standard [stmt.return]), which
is how this unexpected access occurs, but the copy/move may be elided, which
is why behavior is undefined.
copied or moved ([stmt.return]), which is how this unexpected access occurs, but
the copy/move may be elided, which is why behavior is unspecified.

This tip only applies when you are returning a non-reference local variable.
Returning any other expression will not trigger this problem.

## The Problem

There are 2 different optimizations on return statements that can modify the
behavior of our original code snippet: NRVO (See [TotW #11](/tips/11)) and
behavior of that code snippet: the named return value optimization (NRVO) and
implicit moves.

The *before* code worked because copy elision is taking place and the
`return` statement is not actually doing any work. The variable `status` is
already constructed in the return address and the cleanup object is seeing this
unique instance of the `MyStatus` object _after_ the return statement.
The *before* code was working because NRVO is being performed and the `return`
statement is not actually doing any work. The variable `status` is already
constructed in the return address and the cleanup object is seeing this unique
instance of the `Status` object _after_ the return statement.

In the *after* code, copy elision is not being applied and the returned variable
is being moved into the return value. `RunWhenOutOfScope()` runs after the
move operation is done and it is seeing a moved-from `MyStatus` object.
In the *after* code, NRVO is not being performed and the returned variable is
being moved into the return value. The cleanup object is being run after the
move operation is done and it is seeing a moved-from `Status`.

Note that the *before* code was not correct either, as it relies on the copy
elision optimization for correctness. We encourage you to rely on copy elision
for performance (See [TotW #24](/tips/24)), but not correctness. After all,
copy elision is an _optional_ optimization and compiler options or quality of
implementation of the compiler can affect whether or not it happens.
Note that the *before* code was not correct either, as it relies on NRVO for
correctness. We encourage you to rely on NRVO for performance (See TotW #24),
but not correctness. After all, NRVO is an _optional_ optimization and compiler
options or quality of implementation of the compiler can affect whether or not
it happens.

## Solution

Expand All @@ -81,8 +78,8 @@ processing, and one that calls the first one and does the post-processing (ie
logging on error). Eg:

```c++
MyStatus DoSomething() {
MyStatus status;
absl::Status DoSomething() {
absl::Status status;
status = DoA();
if (!status.ok()) return status;
status = DoB();
Expand All @@ -91,26 +88,27 @@ MyStatus DoSomething() {
if (!status.ok()) return status;
return status;
}
MyStatus DoSomethingAndLog() {
MyStatus status = DoSomething();

absl::Status DoSomethingAndLog() {
absl::Status status = DoSomething();
if (!status.ok()) LOG(ERROR) << status;
return status;
}
```

If you are only reading the value, you could also make sure to disable the
optimizations instead. That would force a copy to be made all the time and the
post-processing will not see a moved-from value. E.g.:
post-processing will not see a moved-from value. Eg:

```c++
MyStatus DoSomething() {
MyStatus status_no_nrvo;
absl::Status DoSomething() {
absl::Status status_no_nrvo;
// 'status' is a reference so NRVO and all associated optimizations
// will be disabled.
// The 'return status;' statements will always copy the object and Logger
// will always see the correct value.
MyStatus& status = status_no_nrvo;
auto log_on_error = RunWhenOutOfScope([&status] {
absl::Status& status = status_no_nrvo;
auto log_on_error = absl::MakeCleanup([&status] {
if (!status.ok()) LOG(ERROR) << status;
});
status = DoA();
Expand All @@ -123,19 +121,19 @@ MyStatus DoSomething() {
}
```

## Another example:
## Another Real Life Example

```c++
std::string EncodeVarInt(int i) {
std::string out;
StringOutputStream string_output(&out);
CodedOutputStream coded_output(&string_output);
proto2::io::StringOutputStream string_output(&out);
proto2::io::CodedOutputStream coded_output(&string_output);
coded_output.WriteVarint32(i);
return out;
}
```

`CodedOutputStream` does some work in the destructor to trim unused trailing
`CodedOutputStream` does some work on the destructor to trim unused trailing
bytes. This function can leave garbage bytes on the string if NRVO does not
happen.

Expand All @@ -149,8 +147,8 @@ the block is finished. Like this:
std::string EncodeVarInt(int i) {
std::string out;
{
StringOutputStream string_output(&out);
CodedOutputStream coded_output(&string_output);
proto2::io::StringOutputStream string_output(&out);
proto2::io::CodedOutputStream coded_output(&string_output);
coded_output.WriteVarint32(i);
}
// At this point the streams are destroyed and they already flushed.
Expand Down