C++: FP in cpp/large-parameter with overloaded binary oeprators

[rdmarsh2]

In C++, one pattern for implementing operator+ and other overloaded binary operators is the following:

Type operator+(Type lhs, Type const& rhs ) {
   lhs += rhs;
   return lhs;
}

Exactly how this is compiled depends on both compiler version and optimization level, but it seems not to involve an additional copy at -O2 or -O3 on recent versions of the major compilers. See https://godbolt.org/z/W7BXKk for some experimentation with this.

[geoffw0]

I'm starting work on a PR that will add tests and possibly a fix for this.

[jbj]

To check whether a parameter (or some other variable) may be modified, I think the EscapesTree.qll library can do most of the work. If variableAccessedAsValue(param.getAnAccess(), e.getFullyConverted()) and e is on the LHS of an Assignment or CrementOperation, then the parameter is directly modified. If param.getAnAccess().isAddressOfAccessNonConst(), then the parameter may be modified indirectly because its address escapes.

Using EscapesTree.qll should ensure that complicated expressions and edge cases are handled correctly. For example, x.a[i]++ is a modification of x if it has type struct { int a[8]; } but not if it has type struct { int *a; }.

[geoffw0]

I've created #1279 but I'd like to encourage a bit more discussion around it rather than a quick merge.

I didn't figure out how to use variableAccessedAsValue to any particular advantage.

[jbj]

I recently watched parts of a CppCon talk by Herb Sutter, who is a well-known C++ guru. He advises against passing non-trivial parameters by value since it makes it easier to make certain mistakes and since performance suffers in some important cases.

The advice has made it into the C++ Core Guidelines (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#fcall-parameter-passing), where it's suggested that a static analysis tool should:

Warn when a parameter being passed by value has a size greater than 2 * sizeof(void*). Suggest using a reference to const instead.

Our query is certainly on the conservative side of that.

[geoffw0]

#1279 has been merged and resolves the situation described here. It's true that passing large parameters by value can be considered an undesirable pattern regardless - if anybody has strong feelings about this we could create a non-default query to report the other cases.

[crisluengo]

The CLang linter actually encourages pass by value in these cases. The reasoning is as follows:

struct C {};

C op1(C const& c1, C const& c2) {
   C out = c1;
   out += c2;
   return out;
}

C op2(C c1, C const& c2) {
   c1 += c2;
   return c1;
}

int main() {
   C c1, c2;
   C c3 = op1(c1, c2);  // case 1: call with values on the stack
   C c4 = op1(c1, c2);
   C c5 = op1({}, {});  // case 2: call with temporaries
   C c6 = op2({}, {});
}

In case 1, both op1 and op2 make a copy of c1, add c2 to it, and the result is assigned into a new variable (note C++'s return value optimization, not sure what it's called nowadays, but it's mandated in the standard that return by value does not incur a copy nor a move).

In case 2, op1 makes a copy of the temporary, which is passed by reference, and works just like before. However, op2 takes the temporary, adds the other value to it, and assigns it into c6. In this case there is no 3rd object created ever.

Thus, op2 saves a copy in this case over op1.

Another very common construct in C++ is the "take by value and move" paradigm in constructors:

class LargeData {};

class C {
   C(LargeData data) : data_(std::move(data)) {}
private:
   LargeData data_;
};

The reasoning here is the same. We can construct an object of class C using a temporary value of type LargeData, without actually copying that data.

Whenever a copy needs to be made of data within a function, it is better to let the compiler make the copy of the input argument, because it can avoid the copy if the input value is a temporary.

I believe the right logic is:

• Use the large value only for reading: pass by const reference.
• Make a copy of the large value: pass by value.
• Modify the value: pass by reference.

---

In any case, thank you for implementing this!