Skip to content

Commit fe35192

Browse files
MichaelRFairhurstMichaelRFairhurst
committed
Address feedback
1 parent 6faa015 commit fe35192

File tree

6 files changed

+540
-44
lines changed

6 files changed

+540
-44
lines changed

‎cpp/common/src/codingstandards/cpp/ast/ValueCategory.qll‎

Lines changed: 76 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,18 +4,81 @@ import cpp
44
* Get an expression's value category as a ValueCategory object.
55
*
66
* Note that the standard cpp library exposes `is{_}ValueCategory` predicates, but they do not
7-
* correctly work with conversions. This function is intended to give the correct answer in the
8-
* presence of conversions such as lvalue-to-rvalue conversion.
7+
* necessarily work as expected due to how CodeQL handles reference adjustment and binding, which
8+
* this predicate attempts to handle. There are additional unhandled cases involving
9+
* lvalue-to-rvalue conversions as well.
10+
*
11+
* In C++17, an expression of type "reference to T" is adjusted to type T as stated in [expr]/5.
12+
* This is not a conversion, but in CodeQL this is handled by `ReferenceDereferenceExpr`, a type of
13+
* `Conversion`. Similarly, the binding of references to values is described in [dcl.init.ref],
14+
* which is not a conversion, but in CodeQL this is handled by `ReferenceToExpr`, another type of
15+
* `Conversion`.
16+
*
17+
* Furthermore, the `Expr` predicate `hasLValueToRValueConversion()` uses a different dbscheme table
18+
* than the `Conversion` table, and it is possible to have expressions such that
19+
* `hasLValueToRValueConversion()` holds, but there is no corresponding `Conversion` entry.
20+
*
21+
* Therefore, the value categories of expressions before `ReferenceDereferenceExpr` and after
22+
* `ReferenceToExpr` conversions are therefore essentially unspecified, and the `is{_}ValueCategory`
23+
* predicate results should not be relied upon. And types that are missing a
24+
* `LvalueToRValueConversion` will also return incorrect value categories.
25+
*
26+
* For example, in CodeQL 2.21.4:
27+
*
28+
* ```cpp
29+
* int &i = ...;
30+
* auto r = std::move(i); // 1.) i is a `prvalue(load)` in CodeQL, not an lvalue.
31+
* // 2.) i has a `ReferenceDereferenceExpr` conversion of lvalue category
32+
* // 3.) the conversion from 2 has a `ReferenceToExpr` conversion of prvalue
33+
* // category.
34+
* int i2 = ...;
35+
* f(i2); // 1.) i2 is an lvalue.
36+
* // 2.) i2 undergoes lvalue-to-rvalue conversion, but there is no corresponding `Conversion`.
37+
* // 3.) i2 is treated at a prvalue by CodeQL, but `hasLValueToRValueConversion()` holds.
38+
*
39+
* int get_int();
40+
* auto r2 = std::move(get_int()); // 1.) get_int() itself is treated as a prvalue by CodeQL.
41+
* // 2.) get_int() has a `TemporaryObjectExpr` conversion of lvalue
42+
* // category.
43+
* // 3.) the conversion from 2 has a `ReferenceToExpr` conversion
44+
* // of prvalue category.
45+
* std::string get_str();
46+
* auto r3 = std::move(get_str()); // 1.) get_str() is treated as a prvalue by CodeQL.
47+
* // 2.) get_str() has a `TemporaryObjectExpr` conversion of xvalue
48+
* // 3.) the conversion from 2 has a `ReferenceToExpr` conversion
49+
* // of prvalue category.
50+
* std::string &str_ref();
51+
* auto r3 = std::move(str_ref()); // 1.) str_ref() is treated as a prvalue by CodeQL.
52+
* // 2.) str_ref() has a `ReferenceDereferenceExpr` conversion of
53+
* // lvalue.
54+
* // 3.) the conversion from 2 has a `ReferenceToExpr` conversion
55+
* // of prvalue category.
56+
* ```
57+
*
58+
* As can be seen above, the value categories of expressions are correct between the
59+
* `ReferenceDereferenceExpr` and `ReferenceToExpr`, but not necessarily before or after.
60+
*
61+
* We must also check for `hasLValueToRValueConversion()` and handle that appropriately.
962
*/
1063
ValueCategory getValueCategory(Expr e) {
11-
not exists(e.getConversion()) and
12-
result = getDirectValueCategory(e)
13-
or
14-
if e.getConversion() instanceof ReferenceToExpr
15-
then result = getDirectValueCategory(e)
16-
else result = getDirectValueCategory(e.getConversion())
64+
// If `e` is adjusted from a reference to a value (C++17 [expr]/5) then we want the value category
65+
// of the expression after `ReferenceDereferenceExpr`.
66+
if e.getConversion() instanceof ReferenceDereferenceExpr
67+
then result = getValueCategory(e.getConversion())
68+
else (
69+
// If `hasLValueToRValueConversion()` holds, then ensure we have an lvalue category.
70+
if e.hasLValueToRValueConversion()
71+
then result.isLValue()
72+
else
73+
// Otherwise, get the value category from `is{_}ValueCategory` predicates as normal.
74+
result = getDirectValueCategory(e)
75+
)
1776
}
1877

78+
/**
79+
* Gets the value category of an expression using `is{_}ValueCategory` predicates, without looking
80+
* through conversions.
81+
*/
1982
private ValueCategory getDirectValueCategory(Expr e) {
2083
if e.isLValueCategory()
2184
then result = LValue(e.getValueCategoryString())
@@ -39,6 +102,11 @@ newtype TValueCategory =
39102
exists(Expr e | e.isXValueCategory() and descr = e.getValueCategoryString())
40103
}
41104

105+
/**
106+
* A value category, which can be an lvalue, prvalue, or xvalue.
107+
*
108+
* Note that prvalue has two possible forms: `prvalue` and `prvalue(load)`.
109+
*/
42110
class ValueCategory extends TValueCategory {
43111
string description;
44112

‎cpp/common/test/library/codingstandards/cpp/ast/ValueCategory/ValueCategoryTest.expected‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
| test.cpp:14:3:14:9 | call to get_val | prvalue |
2+
| test.cpp:15:3:15:9 | call to get_ref | lvalue |
3+
| test.cpp:16:3:16:10 | call to get_rref | xvalue |
4+
| test.cpp:19:12:19:14 | val | lvalue |
5+
| test.cpp:20:12:20:14 | val | lvalue |
6+
| test.cpp:21:13:21:16 | call to move | xvalue |
7+
| test.cpp:21:18:21:20 | val | lvalue |
8+
| test.cpp:22:12:22:14 | val | lvalue |
9+
| test.cpp:23:12:23:15 | call to move | xvalue |
10+
| test.cpp:23:17:23:19 | val | lvalue |
11+
| test.cpp:25:12:25:18 | call to get_val | prvalue |
12+
| test.cpp:26:13:26:19 | call to get_val | prvalue |
13+
| test.cpp:27:13:27:16 | call to move | xvalue |
14+
| test.cpp:27:18:27:24 | call to get_val | prvalue |
15+
| test.cpp:28:12:28:18 | call to get_val | prvalue |
16+
| test.cpp:29:12:29:15 | call to move | xvalue |
17+
| test.cpp:29:17:29:23 | call to get_val | prvalue |
18+
| test.cpp:31:14:31:16 | val | lvalue |
19+
| test.cpp:32:12:32:14 | ref | lvalue |
20+
| test.cpp:33:12:33:14 | ref | lvalue |
21+
| test.cpp:34:13:34:16 | call to move | xvalue |
22+
| test.cpp:34:18:34:20 | ref | lvalue |
23+
| test.cpp:35:12:35:14 | ref | lvalue |
24+
| test.cpp:36:12:36:15 | call to move | xvalue |
25+
| test.cpp:36:17:36:19 | ref | lvalue |
26+
| test.cpp:38:12:38:18 | call to get_ref | lvalue |
27+
| test.cpp:39:12:39:18 | call to get_ref | lvalue |
28+
| test.cpp:40:13:40:16 | call to move | xvalue |
29+
| test.cpp:40:18:40:24 | call to get_ref | lvalue |
30+
| test.cpp:41:12:41:18 | call to get_ref | lvalue |
31+
| test.cpp:42:12:42:15 | call to move | xvalue |
32+
| test.cpp:42:17:42:23 | call to get_ref | lvalue |
33+
| test.cpp:44:16:44:19 | call to move | xvalue |
34+
| test.cpp:44:21:44:23 | val | lvalue |
35+
| test.cpp:45:12:45:15 | rref | lvalue |
36+
| test.cpp:46:12:46:15 | rref | lvalue |
37+
| test.cpp:47:13:47:16 | call to move | xvalue |
38+
| test.cpp:47:18:47:21 | rref | lvalue |
39+
| test.cpp:48:12:48:15 | rref | lvalue |
40+
| test.cpp:49:12:49:15 | call to move | xvalue |
41+
| test.cpp:49:17:49:20 | rref | lvalue |
42+
| test.cpp:51:12:51:19 | call to get_rref | lvalue |
43+
| test.cpp:52:13:52:20 | call to get_rref | xvalue |
44+
| test.cpp:53:13:53:16 | call to move | xvalue |
45+
| test.cpp:53:18:53:25 | call to get_rref | xvalue |
46+
| test.cpp:54:12:54:19 | call to get_rref | xvalue |
47+
| test.cpp:55:12:55:15 | call to move | xvalue |
48+
| test.cpp:55:17:55:24 | call to get_rref | xvalue |

‎cpp/common/test/library/codingstandards/cpp/ast/ValueCategory/ValueCategoryTest.ql‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import cpp
2+
import codingstandards.cpp.ast.ValueCategory
3+
4+
predicate isRelevant(Expr e) {
5+
e.(VariableAccess).getTarget().hasName(["val", "ref", "rref"])
6+
or
7+
e.(FunctionCall).getTarget().hasName(["get_val", "get_ref", "get_rref", "move"])
8+
}
9+
10+
from Expr e, ValueCategory cat
11+
where
12+
e.getEnclosingFunction().hasName("main") and
13+
isRelevant(e) and
14+
cat = getValueCategory(e)
15+
select e, cat

‎cpp/common/test/library/codingstandards/cpp/ast/ValueCategory/test.cpp‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
void take_val(int) {}
2+
void take_ref(int &) {}
3+
void take_rref(int &&) {}
4+
template <typename T> void take_uni(T &&) {}
5+
6+
int get_val();
7+
int &get_ref();
8+
int &&get_rref();
9+
10+
template <typename T> T &&move(T &t) { return static_cast<T &&>(t); }
11+
template <typename T> T &&move(T &&t) { return static_cast<T &&>(t); }
12+
13+
int main() {
14+
get_val();
15+
get_ref();
16+
get_rref();
17+
18+
int val = 42;
19+
take_val(val);
20+
take_ref(val);
21+
take_rref(move(val));
22+
take_uni(val);
23+
take_uni(move(val));
24+
25+
take_val(get_val());
26+
take_rref(get_val());
27+
take_rref(move(get_val()));
28+
take_uni(get_val());
29+
take_uni(move(get_val()));
30+
31+
int &ref = val;
32+
take_val(ref);
33+
take_ref(ref);
34+
take_rref(move(ref));
35+
take_uni(ref);
36+
take_uni(move(ref));
37+
38+
take_val(get_ref());
39+
take_ref(get_ref());
40+
take_rref(move(get_ref()));
41+
take_uni(get_ref());
42+
take_uni(move(get_ref()));
43+
44+
int &&rref = move(val);
45+
take_val(rref);
46+
take_ref(rref);
47+
take_rref(move(rref));
48+
take_uni(rref);
49+
take_uni(move(rref));
50+
51+
take_val(get_rref());
52+
take_rref(get_rref());
53+
take_rref(move(get_rref()));
54+
take_uni(get_rref());
55+
take_uni(move(get_rref()));
56+
57+
return 0;
58+
}

0 commit comments

Comments
 (0)