[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.
vsapsai added a comment. Thanks for the review. Explicit `this` usage is a good test, added it. Checked that it happened to work with my previous approach because initializer was `CXXDefaultInitExpr` with expression ImplicitCastExpr 0x7fe966808c40 'void *' `-CXXThisExpr 0x7fe966808c28 'struct A::(anonymous at file.cc:3:5) *' this Repository: rL LLVM https://reviews.llvm.org/D42498 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL325997: [ExprConstant] Fix crash when initialize an indirect
field with another field. (authored by vsapsai, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D42498?vs=135708=135726#toc
Repository:
rL LLVM
https://reviews.llvm.org/D42498
Files:
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
Index: cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
+++ cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1045,3 +1045,57 @@
constexpr B b;
constexpr int p = b.n; // expected-error {{constant expression}} expected-note {{mutable}}
}
+
+namespace IndirectFields {
+
+// Reference indirect field.
+struct A {
+ struct {
+union {
+ int x = x = 3; // expected-note {{outside its lifetime}}
+};
+ };
+ constexpr A() {}
+};
+static_assert(A().x == 3, ""); // expected-error{{not an integral constant expression}} expected-note{{in call to 'A()'}}
+
+// Reference another indirect field, with different 'this'.
+struct B {
+ struct {
+union {
+ int x = 3;
+};
+int y = x;
+ };
+ constexpr B() {}
+};
+static_assert(B().y == 3, "");
+
+// Nested evaluation of indirect field initializers.
+struct C {
+ union {
+int x = 1;
+ };
+};
+struct D {
+ struct {
+C c;
+int y = c.x + 1;
+ };
+};
+static_assert(D().y == 2, "");
+
+// Explicit 'this'.
+struct E {
+ int n = 0;
+ struct {
+void *x = this;
+ };
+ void *y = this;
+};
+constexpr E e1 = E();
+static_assert(e1.x != e1.y, "");
+constexpr E e2 = E{0};
+static_assert(e2.x != e2.y, "");
+
+} // namespace IndirectFields
Index: cfe/trunk/lib/AST/ExprConstant.cpp
===
--- cfe/trunk/lib/AST/ExprConstant.cpp
+++ cfe/trunk/lib/AST/ExprConstant.cpp
@@ -4383,6 +4383,7 @@
#endif
for (const auto *I : Definition->inits()) {
LValue Subobject = This;
+LValue SubobjectParent = This;
APValue *Value =
// Determine the subobject to initialize.
@@ -4413,7 +4414,8 @@
} else if (IndirectFieldDecl *IFD = I->getIndirectMember()) {
// Walk the indirect field decl's chain to find the object to initialize,
// and make sure we've initialized every step along it.
- for (auto *C : IFD->chain()) {
+ auto IndirectFieldChain = IFD->chain();
+ for (auto *C : IndirectFieldChain) {
FD = cast(C);
CXXRecordDecl *CD = cast(FD->getParent());
// Switch the union field if it differs. This happens if we had
@@ -4429,6 +4431,10 @@
*Value = APValue(APValue::UninitStruct(), CD->getNumBases(),
std::distance(CD->field_begin(), CD->field_end()));
}
+// Store Subobject as its parent before updating it for the last element
+// in the chain.
+if (C == IndirectFieldChain.back())
+ SubobjectParent = Subobject;
if (!HandleLValueMember(Info, I->getInit(), Subobject, FD))
return false;
if (CD->isUnion())
@@ -4440,10 +4446,16 @@
llvm_unreachable("unknown base initializer kind");
}
+// Need to override This for implicit field initializers as in this case
+// This refers to innermost anonymous struct/union containing initializer,
+// not to currently constructed class.
+const Expr *Init = I->getInit();
+ThisOverrideRAII ThisOverride(*Info.CurrentCall, ,
+ isa(Init));
FullExpressionRAII InitScope(Info);
-if (!EvaluateInPlace(*Value, Info, Subobject, I->getInit()) ||
-(FD && FD->isBitField() && !truncateBitfieldValue(Info, I->getInit(),
- *Value, FD))) {
+if (!EvaluateInPlace(*Value, Info, Subobject, Init) ||
+(FD && FD->isBitField() &&
+ !truncateBitfieldValue(Info, Init, *Value, FD))) {
// If we're checking for a potential constant expression, evaluate all
// initializers even if some of them fail.
if (!Info.noteFailure())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC325997: [ExprConstant] Fix crash when initialize an indirect
field with another field. (authored by vsapsai, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D42498?vs=135708=135725#toc
Repository:
rL LLVM
https://reviews.llvm.org/D42498
Files:
lib/AST/ExprConstant.cpp
test/SemaCXX/constant-expression-cxx1y.cpp
Index: test/SemaCXX/constant-expression-cxx1y.cpp
===
--- test/SemaCXX/constant-expression-cxx1y.cpp
+++ test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1045,3 +1045,57 @@
constexpr B b;
constexpr int p = b.n; // expected-error {{constant expression}} expected-note {{mutable}}
}
+
+namespace IndirectFields {
+
+// Reference indirect field.
+struct A {
+ struct {
+union {
+ int x = x = 3; // expected-note {{outside its lifetime}}
+};
+ };
+ constexpr A() {}
+};
+static_assert(A().x == 3, ""); // expected-error{{not an integral constant expression}} expected-note{{in call to 'A()'}}
+
+// Reference another indirect field, with different 'this'.
+struct B {
+ struct {
+union {
+ int x = 3;
+};
+int y = x;
+ };
+ constexpr B() {}
+};
+static_assert(B().y == 3, "");
+
+// Nested evaluation of indirect field initializers.
+struct C {
+ union {
+int x = 1;
+ };
+};
+struct D {
+ struct {
+C c;
+int y = c.x + 1;
+ };
+};
+static_assert(D().y == 2, "");
+
+// Explicit 'this'.
+struct E {
+ int n = 0;
+ struct {
+void *x = this;
+ };
+ void *y = this;
+};
+constexpr E e1 = E();
+static_assert(e1.x != e1.y, "");
+constexpr E e2 = E{0};
+static_assert(e2.x != e2.y, "");
+
+} // namespace IndirectFields
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -4383,6 +4383,7 @@
#endif
for (const auto *I : Definition->inits()) {
LValue Subobject = This;
+LValue SubobjectParent = This;
APValue *Value =
// Determine the subobject to initialize.
@@ -4413,7 +4414,8 @@
} else if (IndirectFieldDecl *IFD = I->getIndirectMember()) {
// Walk the indirect field decl's chain to find the object to initialize,
// and make sure we've initialized every step along it.
- for (auto *C : IFD->chain()) {
+ auto IndirectFieldChain = IFD->chain();
+ for (auto *C : IndirectFieldChain) {
FD = cast(C);
CXXRecordDecl *CD = cast(FD->getParent());
// Switch the union field if it differs. This happens if we had
@@ -4429,6 +4431,10 @@
*Value = APValue(APValue::UninitStruct(), CD->getNumBases(),
std::distance(CD->field_begin(), CD->field_end()));
}
+// Store Subobject as its parent before updating it for the last element
+// in the chain.
+if (C == IndirectFieldChain.back())
+ SubobjectParent = Subobject;
if (!HandleLValueMember(Info, I->getInit(), Subobject, FD))
return false;
if (CD->isUnion())
@@ -4440,10 +4446,16 @@
llvm_unreachable("unknown base initializer kind");
}
+// Need to override This for implicit field initializers as in this case
+// This refers to innermost anonymous struct/union containing initializer,
+// not to currently constructed class.
+const Expr *Init = I->getInit();
+ThisOverrideRAII ThisOverride(*Info.CurrentCall, ,
+ isa(Init));
FullExpressionRAII InitScope(Info);
-if (!EvaluateInPlace(*Value, Info, Subobject, I->getInit()) ||
-(FD && FD->isBitField() && !truncateBitfieldValue(Info, I->getInit(),
- *Value, FD))) {
+if (!EvaluateInPlace(*Value, Info, Subobject, Init) ||
+(FD && FD->isBitField() &&
+ !truncateBitfieldValue(Info, Init, *Value, FD))) {
// If we're checking for a potential constant expression, evaluate all
// initializers even if some of them fail.
if (!Info.noteFailure())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Looks good, thanks!
In https://reviews.llvm.org/D42498#1015419, @vsapsai wrote:
> In https://reviews.llvm.org/D42498#1007028, @rsmith wrote:
>
> > […] your approach will still go wrong if the `This` pointer is used in
> > other ways, such as an explicit mention of `this` or a member function call.
>
>
> I wasn't able to find a failing test case. Maybe I misunderstood you but in
> my testing functions declared in named structs had correct `This` pointer and
> in anonymous structs functions cannot be declared.
Here's a testcase that fails today:
struct A { int n = 0; struct { void *p = this; }; void *q = this; };
constexpr A a = A();
static_assert(a.p != a.q, ""); // fails today, should pass
constexpr A b = A{0};
static_assert(b.p != b.q, ""); // passes today
With the previous approach of fixing up the `This` value only in class member
access, the above testcase would presumably still fail. I expect it to pass
with your new approach.
https://reviews.llvm.org/D42498
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.
vsapsai updated this revision to Diff 135708.
vsapsai added a comment.
- Override `This` for indirect field initializer more like it is done for
`InitListExpr`.
I rebased my changes, so the diff between different versions can be noisy.
https://reviews.llvm.org/D42498
Files:
clang/lib/AST/ExprConstant.cpp
clang/test/SemaCXX/constant-expression-cxx1y.cpp
Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1044,3 +1044,44 @@
constexpr B b;
constexpr int p = b.n; // expected-error {{constant expression}} expected-note {{mutable}}
}
+
+namespace IndirectFields {
+
+// Reference indirect field.
+struct A {
+ struct {
+union {
+ int x = x = 3; // expected-note {{outside its lifetime}}
+};
+ };
+ constexpr A() {}
+};
+static_assert(A().x == 3, ""); // expected-error{{not an integral constant expression}} expected-note{{in call to 'A()'}}
+
+// Reference another indirect field, with different 'this'.
+struct B {
+ struct {
+union {
+ int x = 3;
+};
+int y = x;
+ };
+ constexpr B() {}
+};
+static_assert(B().y == 3, "");
+
+// Nested evaluation of indirect field initializers.
+struct C {
+ union {
+int x = 1;
+ };
+};
+struct D {
+ struct {
+C c;
+int y = c.x + 1;
+ };
+};
+static_assert(D().y == 2, "");
+
+} // namespace IndirectFields
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -4383,6 +4383,7 @@
#endif
for (const auto *I : Definition->inits()) {
LValue Subobject = This;
+LValue SubobjectParent = This;
APValue *Value =
// Determine the subobject to initialize.
@@ -4413,7 +4414,8 @@
} else if (IndirectFieldDecl *IFD = I->getIndirectMember()) {
// Walk the indirect field decl's chain to find the object to initialize,
// and make sure we've initialized every step along it.
- for (auto *C : IFD->chain()) {
+ auto IndirectFieldChain = IFD->chain();
+ for (auto *C : IndirectFieldChain) {
FD = cast(C);
CXXRecordDecl *CD = cast(FD->getParent());
// Switch the union field if it differs. This happens if we had
@@ -4429,6 +4431,10 @@
*Value = APValue(APValue::UninitStruct(), CD->getNumBases(),
std::distance(CD->field_begin(), CD->field_end()));
}
+// Store Subobject as its parent before updating it for the last element
+// in the chain.
+if (C == IndirectFieldChain.back())
+ SubobjectParent = Subobject;
if (!HandleLValueMember(Info, I->getInit(), Subobject, FD))
return false;
if (CD->isUnion())
@@ -4440,10 +4446,16 @@
llvm_unreachable("unknown base initializer kind");
}
+// Need to override This for implicit field initializers as in this case
+// This refers to innermost anonymous struct/union containing initializer,
+// not to currently constructed class.
+const Expr *Init = I->getInit();
+ThisOverrideRAII ThisOverride(*Info.CurrentCall, ,
+ isa(Init));
FullExpressionRAII InitScope(Info);
-if (!EvaluateInPlace(*Value, Info, Subobject, I->getInit()) ||
-(FD && FD->isBitField() && !truncateBitfieldValue(Info, I->getInit(),
- *Value, FD))) {
+if (!EvaluateInPlace(*Value, Info, Subobject, Init) ||
+(FD && FD->isBitField() &&
+ !truncateBitfieldValue(Info, Init, *Value, FD))) {
// If we're checking for a potential constant expression, evaluate all
// initializers even if some of them fail.
if (!Info.noteFailure())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.
vsapsai added a comment. Thanks for response, Richard. I'll look into using `CXXDefaultInitExpr`. As for In https://reviews.llvm.org/D42498#1007028, @rsmith wrote: > […] your approach will still go wrong if the `This` pointer is used in other > ways, such as an explicit mention of `this` or a member function call. I wasn't able to find a failing test case. Maybe I misunderstood you but in my testing functions declared in named structs had correct `This` pointer and in anonymous structs functions cannot be declared. https://reviews.llvm.org/D42498 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.
rsmith added a comment. Please handle this by temporarily updating the `This` pointer appropriately when evaluating the `CXXDefaultInitExpr` rather than trying to work around the `This` value being wrong later (your approach will still go wrong if the `This` pointer is used in other ways, such as an explicit mention of `this` or a member function call). You can refer to how we do this in CodeGen: look for `CodeGenFunction::CXXDefaultInitExprScope` and `CodeGenFunction::FieldConstructionScope`. https://reviews.llvm.org/D42498 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.
vsapsai added a comment. Herald added a subscriber: jkorous-apple. Ping. https://reviews.llvm.org/D42498 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42498: [ExprConstant] Fix crash when initialize an indirect field with another field.
vsapsai created this revision.
vsapsai added reviewers: rsmith, efriedma.
When indirect field is initialized with another field, you have
MemberExpr with CXXThisExpr that corresponds to the field's immediate
anonymous parent. But 'this' was referring to the non-anonymous parent.
So when we were building LValue Designator, it was incorrect as it had
wrong starting point. Usage of such designator would cause unexpected
APValue changes and crashes.
The fix is in adjusting 'this' for indirect fields from non-anonymous
parent to the field's immediate parent.
Discovered by OSS-Fuzz:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=4985
rdar://problem/36359187
https://reviews.llvm.org/D42498
Files:
clang/lib/AST/ExprConstant.cpp
clang/test/SemaCXX/constant-expression-cxx1y.cpp
Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1021,3 +1021,44 @@
}
static_assert(evalNested(), "");
} // namespace PR19741
+
+namespace IndirectFields {
+
+// Reference indirect field.
+struct A {
+ struct {
+union {
+ int x = x = 3; // expected-note {{outside its lifetime}}
+};
+ };
+ constexpr A() {}
+};
+static_assert(A().x == 3, ""); // expected-error{{not an integral constant expression}} expected-note{{in call to 'A()'}}
+
+// Reference another indirect field, with different 'this'.
+struct B {
+ struct {
+union {
+ int x = 3;
+};
+int y = x;
+ };
+ constexpr B() {}
+};
+static_assert(B().y == 3, "");
+
+// Nested evaluation of indirect field initializers.
+struct C {
+ union {
+int x = 1;
+ };
+};
+struct D {
+ struct {
+C c;
+int y = c.x + 1;
+ };
+};
+static_assert(D().y == 2, "");
+
+} // namespace IndirectFields
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -449,6 +449,9 @@
/// Index - The call index of this call.
unsigned Index;
+/// IndirectField - The indirect field being evaluated.
+const IndirectFieldDecl *IndirectField = nullptr;
+
// FIXME: Adding this to every 'CallStackFrame' may have a nontrivial impact
// on the overall stack usage of deeply-recursing constexpr evaluataions.
// (We should cache this map rather than recomputing it repeatedly.)
@@ -4370,6 +4373,7 @@
for (const auto *I : Definition->inits()) {
LValue Subobject = This;
APValue *Value =
+Frame.IndirectField = nullptr;
// Determine the subobject to initialize.
FieldDecl *FD = nullptr;
@@ -4399,6 +4403,7 @@
} else if (IndirectFieldDecl *IFD = I->getIndirectMember()) {
// Walk the indirect field decl's chain to find the object to initialize,
// and make sure we've initialized every step along it.
+ Frame.IndirectField = IFD;
for (auto *C : IFD->chain()) {
FD = cast(C);
CXXRecordDecl *CD = cast(FD->getParent());
@@ -4437,6 +4442,7 @@
Success = false;
}
}
+ Frame.IndirectField = nullptr;
return Success &&
EvaluateStmt(Ret, Info, Definition->getBody()) != ESR_Failed;
@@ -5612,6 +5618,20 @@
Result.setFrom(Info.Ctx, RVal);
}
+} else if (Info.CurrentCall->IndirectField) {
+ // Fields of anonymous structs/unions can refer to other fields. In this
+ // case the 'this' expression corresponds to anonymous struct/union but
+ // Info.CurrentCall->This corresponds to the field's non-anonymous parent.
+ for (auto *C : Info.CurrentCall->IndirectField->chain()) {
+const FieldDecl *FD = cast(C);
+if (!FD->isImplicit()) {
+ assert(E->getType()->getPointeeCXXRecordDecl() == FD->getParent() &&
+ "Should adjust LValue to refer to the same 'this'");
+ break;
+}
+if (!HandleLValueMember(Info, E, Result, FD))
+ return false;
+ }
}
return true;
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
