You can not select more than 25 topics Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.

207 lines
3.5 KiB

/*
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
[clang] fix bad interaction between ConditionalOperator and initializers Summary: This is several inter-connected changes together to keep the tests happy. The ConditionalOperator `b?t:e` is translated by first creating a placeholder variable to temporarily store the result of the evaluation in each branch, then the real thing we want to assign to reads that variable. But, there are situations where that changes the semantics of the expression, namely when the value created is a struct on the stack (eg, a C++ temporary). This is because in SIL we cannot assign the *address* of a program variable, only its contents, so by the time we're out of the conditional operator we cannot set the struct value correctly anymore: we can only set its content, which we did, but that results in a "shifted" struct value that is one dereference away from where it should be. So a batch of changes concern `conditionalOperator_trans`: - instead of systematically creating a temporary for the conditional, use the `trans_state.var_exp_typ` provided from above if available when translating `ConditionalOperator` - don't even set anything if that variable was already initialized by merely translating the branch expression, eg when it's a constructor - fix long-standing TODO to propagate these initialization facts accurately for ConditionalOperator (used by `init_expr_trans` to also figure out if it should insert a store to the variable being initialised or not) The rest of the changes adapt some relevant other constructs to deal with conditionalOperator properly now that it can set the current variable itself, instead of storing stuff inside a temp variable. This change was a problem because some constructs, eg a variable declaration, will insert nodes that set up the variable before calling its initialization, and now the initialization happens *before* that setup, in the translation of the inner conditional operator, which naturally creates nodes above the current one. - add a generic helper to force a sequential order between two translation results, forcing node creation if necessary - use that in `init_expr_trans` and `cxxNewExpr_trans` - adjust many places where `var_exp_typ` was incorrectly not reset when translating sub-expressions The sequentiality business creates more nodes when used, and the conditionalOperator business uses fewer temporary variables, so the frontend results change quite a bit. Note that biabduction tests were invaluable in debugging this. There could be other constructs to adjust similarly to cxxNewExpr that were not covered by the tests though. Added tests in pulse that exercises the previous bug. Reviewed By: da319 Differential Revision: D24796282 fbshipit-source-id: 0790c8d17
4 years ago
#include <string>
namespace frontend {
namespace some {
namespace thing {
using foo_int = int;
int* bad_ptr() {
int* p = new (int);
delete p;
return p;
}
} // namespace thing
} // namespace some
// test that NamespaceAliasDecl is handled correctly
void deref_null_namespace_alias_ptr_bad() {
namespace st = some::thing;
st::foo_int x = 0;
int* p = st::bad_ptr();
*p = x;
}
[clang] fix bad interaction between ConditionalOperator and initializers Summary: This is several inter-connected changes together to keep the tests happy. The ConditionalOperator `b?t:e` is translated by first creating a placeholder variable to temporarily store the result of the evaluation in each branch, then the real thing we want to assign to reads that variable. But, there are situations where that changes the semantics of the expression, namely when the value created is a struct on the stack (eg, a C++ temporary). This is because in SIL we cannot assign the *address* of a program variable, only its contents, so by the time we're out of the conditional operator we cannot set the struct value correctly anymore: we can only set its content, which we did, but that results in a "shifted" struct value that is one dereference away from where it should be. So a batch of changes concern `conditionalOperator_trans`: - instead of systematically creating a temporary for the conditional, use the `trans_state.var_exp_typ` provided from above if available when translating `ConditionalOperator` - don't even set anything if that variable was already initialized by merely translating the branch expression, eg when it's a constructor - fix long-standing TODO to propagate these initialization facts accurately for ConditionalOperator (used by `init_expr_trans` to also figure out if it should insert a store to the variable being initialised or not) The rest of the changes adapt some relevant other constructs to deal with conditionalOperator properly now that it can set the current variable itself, instead of storing stuff inside a temp variable. This change was a problem because some constructs, eg a variable declaration, will insert nodes that set up the variable before calling its initialization, and now the initialization happens *before* that setup, in the translation of the inner conditional operator, which naturally creates nodes above the current one. - add a generic helper to force a sequential order between two translation results, forcing node creation if necessary - use that in `init_expr_trans` and `cxxNewExpr_trans` - adjust many places where `var_exp_typ` was incorrectly not reset when translating sub-expressions The sequentiality business creates more nodes when used, and the conditionalOperator business uses fewer temporary variables, so the frontend results change quite a bit. Note that biabduction tests were invaluable in debugging this. There could be other constructs to adjust similarly to cxxNewExpr that were not covered by the tests though. Added tests in pulse that exercises the previous bug. Reviewed By: da319 Differential Revision: D24796282 fbshipit-source-id: 0790c8d17
4 years ago
struct X {
int f;
X() {}
X(int i) : f(i) {}
X(X& from) : f(from.f) {}
X(X&& from) : f(from.f) {}
~X() {}
int get_f() const { return f; }
};
void construct_in_conditional_ok() {
if (X(44).f != 44) {
int* p = nullptr;
*p = 42;
}
}
bool is_zero(const X& x) { return x.get_f() == 0; }
void temp_passed_in_conditional_ok() {
X x{44};
if (is_zero(x)) {
int* p = nullptr;
*p = 42;
}
}
void conditional_construction_xvalue_ok() {
X x = true ? X(44) : X(33);
if (x.f != 44) {
int* p = nullptr;
*p = 42;
}
}
void conditional_construction_lvalue_ok() {
const X& x = true ? X(44) : X(33);
if (x.f != 44) {
int* p = nullptr;
*p = 42;
}
}
void conditional_construction_int_lvalue_ok() {
const int& x = true ? 44 : 33;
if (x != 44) {
int* p = nullptr;
*p = 42;
}
}
void conditional_construction_int_ptr_ok() {
int* p = nullptr;
int j = 44;
int* x = true ? &j : p;
int* y = true ? p : &j;
if (*x != 44 || y != nullptr) {
int* p = nullptr;
*p = 42;
}
}
int conditional_expression_bad(bool b) {
bool ok = false;
ok = ok && b;
if (!ok) {
int* p = nullptr;
*p = 42;
}
}
class Frontend {
public:
int a;
Frontend(int n) { a = (n == 0) ? 0 : 1; }
Frontend(Frontend x, int n) {
x.a = (n == 0) ? 0 : 1;
a = x.a;
}
void set_field_via_local(int n) {
int* b = (int*)malloc(sizeof(int));
if (b) {
*b = 0;
*b = (n == 0) ? 0 : 1;
a = *b;
free(b);
} else {
a = (n == 0) ? 0 : 1;
}
}
};
void call_Frontend_constructor_ok() {
Frontend x = Frontend(10); // x.a is 1
if (x.a != 1) {
int* p = nullptr;
*p = 42;
}
}
void call_Frontend_constructor_bad() {
Frontend x = Frontend(10); // x.a is 1
if (x.a == 1) {
int* p = nullptr;
*p = 42;
}
}
void call_Frontend_constructor2_ok() {
Frontend x = Frontend(0); // x.a is 0
Frontend y = Frontend(x, 10); // y.a is 1
if (y.a != 1) {
int* p = nullptr;
*p = 42;
}
}
void call_Frontend_constructor2_bad() {
Frontend x = Frontend(0); // x.a is 0
Frontend y = Frontend(x, 10); // y.a is 1
if (y.a == 1) {
int* p = nullptr;
*p = 42;
}
}
void call_set_field_via_local_ok() {
Frontend x = Frontend(0); // x.a is 0
x.set_field_via_local(10); // x.a is 1
if (x.a != 1) {
int* p = nullptr;
*p = 42;
}
}
void call_set_field_via_local_bad() {
Frontend x = Frontend(0); // x.a is 0
x.set_field_via_local(10); // x.a is 1
if (x.a == 1) {
int* p = nullptr;
*p = 42;
}
}
void not_boolean_ok() {
bool t = true;
bool* b = (bool*)malloc(sizeof(bool));
if (b) {
*b = true;
*b = !t; // *b is false
if (*b) {
int* p = nullptr;
*p = 42;
}
free(b);
}
}
void not_boolean_bad() {
bool f = false;
bool* b = (bool*)malloc(sizeof(bool));
if (b) {
*b = false;
*b = !f; // *b is true
if (*b) {
int* p = nullptr;
*p = 42;
}
free(b);
}
}
[clang] fix bad interaction between ConditionalOperator and initializers Summary: This is several inter-connected changes together to keep the tests happy. The ConditionalOperator `b?t:e` is translated by first creating a placeholder variable to temporarily store the result of the evaluation in each branch, then the real thing we want to assign to reads that variable. But, there are situations where that changes the semantics of the expression, namely when the value created is a struct on the stack (eg, a C++ temporary). This is because in SIL we cannot assign the *address* of a program variable, only its contents, so by the time we're out of the conditional operator we cannot set the struct value correctly anymore: we can only set its content, which we did, but that results in a "shifted" struct value that is one dereference away from where it should be. So a batch of changes concern `conditionalOperator_trans`: - instead of systematically creating a temporary for the conditional, use the `trans_state.var_exp_typ` provided from above if available when translating `ConditionalOperator` - don't even set anything if that variable was already initialized by merely translating the branch expression, eg when it's a constructor - fix long-standing TODO to propagate these initialization facts accurately for ConditionalOperator (used by `init_expr_trans` to also figure out if it should insert a store to the variable being initialised or not) The rest of the changes adapt some relevant other constructs to deal with conditionalOperator properly now that it can set the current variable itself, instead of storing stuff inside a temp variable. This change was a problem because some constructs, eg a variable declaration, will insert nodes that set up the variable before calling its initialization, and now the initialization happens *before* that setup, in the translation of the inner conditional operator, which naturally creates nodes above the current one. - add a generic helper to force a sequential order between two translation results, forcing node creation if necessary - use that in `init_expr_trans` and `cxxNewExpr_trans` - adjust many places where `var_exp_typ` was incorrectly not reset when translating sub-expressions The sequentiality business creates more nodes when used, and the conditionalOperator business uses fewer temporary variables, so the frontend results change quite a bit. Note that biabduction tests were invaluable in debugging this. There could be other constructs to adjust similarly to cxxNewExpr that were not covered by the tests though. Added tests in pulse that exercises the previous bug. Reviewed By: da319 Differential Revision: D24796282 fbshipit-source-id: 0790c8d17
4 years ago
} // namespace frontend