[PATCH] D29469: Fix PR31843: Clang-4.0 crashes/assert while evaluating __builtin_object_size
This revision was automatically updated to reflect the committed changes.
Closed by commit rL294800: Don't let EvaluationModes dictate whether an invalid
base is OK (authored by gbiv).
Changed prior to commit:
https://reviews.llvm.org/D29469?vs=86870=88061#toc
Repository:
rL LLVM
https://reviews.llvm.org/D29469
Files:
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/test/CodeGen/object-size.c
cfe/trunk/test/Sema/builtin-object-size.c
Index: cfe/trunk/lib/AST/ExprConstant.cpp
===
--- cfe/trunk/lib/AST/ExprConstant.cpp
+++ cfe/trunk/lib/AST/ExprConstant.cpp
@@ -616,10 +616,12 @@
/// gets a chance to look at it.
EM_PotentialConstantExpressionUnevaluated,
- /// Evaluate as a constant expression. Continue evaluating if either:
- /// - We find a MemberExpr with a base that can't be evaluated.
- /// - We find a variable initialized with a call to a function that has
- /// the alloc_size attribute on it.
+ /// Evaluate as a constant expression. In certain scenarios, if:
+ /// - we find a MemberExpr with a base that can't be evaluated, or
+ /// - we find a variable initialized with a call to a function that has
+ /// the alloc_size attribute on it
+ /// then we may consider evaluation to have succeeded.
+ ///
/// In either case, the LValue returned shall have an invalid base; in the
/// former, the base will be the invalid MemberExpr, in the latter, the
/// base will be either the alloc_size CallExpr or a CastExpr wrapping
@@ -902,10 +904,6 @@
return KeepGoing;
}
-bool allowInvalidBaseExpr() const {
- return EvalMode == EM_OffsetFold;
-}
-
class ArrayInitLoopIndex {
EvalInfo
uint64_t OuterIndex;
@@ -1416,8 +1414,10 @@
static bool EvaluateInPlace(APValue , EvalInfo ,
const LValue , const Expr *E,
bool AllowNonLiteralTypes = false);
-static bool EvaluateLValue(const Expr *E, LValue , EvalInfo );
-static bool EvaluatePointer(const Expr *E, LValue , EvalInfo );
+static bool EvaluateLValue(const Expr *E, LValue , EvalInfo ,
+ bool InvalidBaseOK = false);
+static bool EvaluatePointer(const Expr *E, LValue , EvalInfo ,
+bool InvalidBaseOK = false);
static bool EvaluateMemberPointer(const Expr *E, MemberPtr ,
EvalInfo );
static bool EvaluateTemporary(const Expr *E, LValue , EvalInfo );
@@ -4835,17 +4835,23 @@
: public ExprEvaluatorBase {
protected:
LValue
+ bool InvalidBaseOK;
typedef LValueExprEvaluatorBase LValueExprEvaluatorBaseTy;
typedef ExprEvaluatorBase ExprEvaluatorBaseTy;
bool Success(APValue::LValueBase B) {
Result.set(B);
return true;
}
+ bool evaluatePointer(const Expr *E, LValue ) {
+return EvaluatePointer(E, Result, this->Info, InvalidBaseOK);
+ }
+
public:
- LValueExprEvaluatorBase(EvalInfo , LValue ) :
-ExprEvaluatorBaseTy(Info), Result(Result) {}
+ LValueExprEvaluatorBase(EvalInfo , LValue , bool InvalidBaseOK)
+ : ExprEvaluatorBaseTy(Info), Result(Result),
+InvalidBaseOK(InvalidBaseOK) {}
bool Success(const APValue , const Expr *E) {
Result.setFrom(this->Info.Ctx, V);
@@ -4857,7 +4863,7 @@
QualType BaseTy;
bool EvalOK;
if (E->isArrow()) {
- EvalOK = EvaluatePointer(E->getBase(), Result, this->Info);
+ EvalOK = evaluatePointer(E->getBase(), Result);
BaseTy = E->getBase()->getType()->castAs()->getPointeeType();
} else if (E->getBase()->isRValue()) {
assert(E->getBase()->getType()->isRecordType());
@@ -4868,7 +4874,7 @@
BaseTy = E->getBase()->getType();
}
if (!EvalOK) {
- if (!this->Info.allowInvalidBaseExpr())
+ if (!InvalidBaseOK)
return false;
Result.setInvalid(E);
return true;
@@ -4962,8 +4968,8 @@
class LValueExprEvaluator
: public LValueExprEvaluatorBase {
public:
- LValueExprEvaluator(EvalInfo , LValue ) :
-LValueExprEvaluatorBaseTy(Info, Result) {}
+ LValueExprEvaluator(EvalInfo , LValue , bool InvalidBaseOK) :
+LValueExprEvaluatorBaseTy(Info, Result, InvalidBaseOK) {}
bool VisitVarDecl(const Expr *E, const VarDecl *VD);
bool VisitUnaryPreIncDec(const UnaryOperator *UO);
@@ -5016,10 +5022,11 @@
/// * function designators in C, and
/// * "extern void" objects
/// * @selector() expressions in Objective-C
-static bool EvaluateLValue(const Expr *E, LValue , EvalInfo ) {
+static bool EvaluateLValue(const Expr *E, LValue , EvalInfo ,
+ bool InvalidBaseOK) {
assert(E->isGLValue() || E->getType()->isFunctionType() ||
E->getType()->isVoidType() || isa(E));
- return LValueExprEvaluator(Info, Result).Visit(E);
+ return LValueExprEvaluator(Info, Result, InvalidBaseOK).Visit(E);
}
bool
[PATCH] D29469: Fix PR31843: Clang-4.0 crashes/assert while evaluating __builtin_object_size
george.burgess.iv marked an inline comment as done. george.burgess.iv added a comment. Thanks! https://reviews.llvm.org/D29469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29469: Fix PR31843: Clang-4.0 crashes/assert while evaluating __builtin_object_size
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM, I think this is also OK for Clang 4 if Hans is willing to take it. Comment at: lib/AST/ExprConstant.cpp:607-612 + /// Evaluate as a constant expression. In certain scenarios, if: + /// - We find a MemberExpr with a base that can't be evaluated, or /// - We find a variable initialized with a call to a function that has - /// the alloc_size attribute on it. + /// the alloc_size attribute on it + /// + /// Then we may consider evaluation to have succeeded. [nit] "We", "We", "Then" should not be capitalized, and please remove blank line prior to "Then we [...]" https://reviews.llvm.org/D29469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29469: Fix PR31843: Clang-4.0 crashes/assert while evaluating __builtin_object_size
dexonsmith added a reviewer: nlopes. dexonsmith added a comment. +Nuno, who worked on objectsize in the past. https://reviews.llvm.org/D29469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29469: Fix PR31843: Clang-4.0 crashes/assert while evaluating __builtin_object_size
george.burgess.iv added a comment. End of week ping :) https://reviews.llvm.org/D29469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29469: Fix PR31843: Clang-4.0 crashes/assert while evaluating __builtin_object_size
mehdi_amini added a comment. Ping? This is hitting 4.0 https://reviews.llvm.org/D29469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29469: Fix PR31843: Clang-4.0 crashes/assert while evaluating __builtin_object_size
mehdi_amini added a comment. > My concern with that approach is that we'd just end up playing whack-a-bug. > It's possible that this fix puts us in a similar situation, but I'm honestly > at a loss for a better approach. ¯\_(ツ)_/¯ (...And I'd kinda prefer to play > whack-a-bug with "clang could do a better job in this case" reports, rather > than "clang crashes on my code" reports.) Agree, thanks :) This patch LGTM but I have little confidence in myself in this area of the compiler, so I rather have someone else stamping it! https://reviews.llvm.org/D29469 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29469: Fix PR31843: Clang-4.0 crashes/assert while evaluating __builtin_object_size
george.burgess.iv created this revision.
We're currently using a special EvaluationMode to determine whether
we're OK with invalid base expressions during objectsize evaluation.
Using it to figure out how we handle UB/etc. is fine, but I think it's
too far-reaching to use for checking whether we're OK with an invalid
base expression. This is because an EvaluationMode applies by default to
all subexpressions of the expression we're evaluating, which causes
issues like in https://llvm.org/bugs/show_bug.cgi?id=31843 .
What I think we actually want to do is allow this relaxed behavior only
for "top-level" member/pointer expressions. In other words, we should
only allow it when we're evaluating the top-level pointers/lvalues
involved in an expression. As soon as we need to evaluate something else
(an int, ...), we should drop these relaxed rules and go back to
stricter evaluation. For example, in:
foo(whatever->a ? b() : 1)->bar->baz.quux
We don't care if `whatever->a` is evaluated using these relaxed rules,
since the only invalid base that's actually useful to the objectsize
evaluator is `foo(...)->bar`.
...As a lower level issue, one idea I had to make this less awkward was
to just define `EvaluatePointer(const Expr *, LValue&, EvalInfo&)` as a
method on LValueExprEvaluator/PointerExprEvaluator that called
::EvaluatePointer with the right fourth arg, but that seemed a bit too
subtle to me. Happy to swap to it if you think it's better.
Finally, if we make this change, ISTM we can just replace OffsetFold
with ConstantFold. I'd rather to that in another patch, since whatever
we choose to do here gets put into 4.0.
--
With all of that said, if we want the minimal fix for PR31843, it's
adding an `InvalidBase` check or two to Evaluate. My concern with that
approach is that we'd just end up playing whack-a-bug. It's possible
that this fix puts us in a similar situation, but I'm honestly at a loss
for a better approach. ¯\_(ツ)_/¯ (...And I'd kinda prefer to play
whack-a-bug with "clang could do a better job in this case" reports,
rather than "clang crashes on my code" reports.)
https://reviews.llvm.org/D29469
Files:
lib/AST/ExprConstant.cpp
test/CodeGen/object-size.c
test/Sema/builtin-object-size.c
Index: test/Sema/builtin-object-size.c
===
--- test/Sema/builtin-object-size.c
+++ test/Sema/builtin-object-size.c
@@ -76,3 +76,18 @@
a += __builtin_object_size(p3->b, 0);
return a;
}
+
+int pr31843() {
+ int n = 0;
+
+ struct { int f; } a;
+ int b;
+ n += __builtin_object_size(({&(b ? : )->f; pr31843;}), 0); // expected-warning{{expression result unused}}
+
+ struct statfs { char f_mntonname[1024];};
+ struct statfs *outStatFSBuf;
+ n += __builtin_object_size(outStatFSBuf->f_mntonname ? "" : "", 1); // expected-warning{{address of array}}
+ n += __builtin_object_size(outStatFSBuf->f_mntonname ?: "", 1);
+
+ return n;
+}
Index: test/CodeGen/object-size.c
===
--- test/CodeGen/object-size.c
+++ test/CodeGen/object-size.c
@@ -549,3 +549,22 @@
// CHECK: store i32 0
gi = __builtin_object_size(incomplete_char_array, 3);
}
+
+// Flips between the pointer and lvalue evaluator a lot.
+void deeply_nested() {
+ struct {
+struct {
+ struct {
+struct {
+ int e[2];
+ char f; // Inhibit our writing-off-the-end check
+} d[2];
+ } c[2];
+} b[2];
+ } *a;
+
+ // CHECK: store i32 4
+ gi = __builtin_object_size(>b[1].c[1].d[1].e[1], 1);
+ // CHECK: store i32 4
+ gi = __builtin_object_size(>b[1].c[1].d[1].e[1], 3);
+}
Index: lib/AST/ExprConstant.cpp
===
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -604,10 +604,13 @@
/// gets a chance to look at it.
EM_PotentialConstantExpressionUnevaluated,
- /// Evaluate as a constant expression. Continue evaluating if either:
- /// - We find a MemberExpr with a base that can't be evaluated.
+ /// Evaluate as a constant expression. In certain scenarios, if:
+ /// - We find a MemberExpr with a base that can't be evaluated, or
/// - We find a variable initialized with a call to a function that has
- /// the alloc_size attribute on it.
+ /// the alloc_size attribute on it
+ ///
+ /// Then we may consider evaluation to have succeeded.
+ ///
/// In either case, the LValue returned shall have an invalid base; in the
/// former, the base will be the invalid MemberExpr, in the latter, the
/// base will be either the alloc_size CallExpr or a CastExpr wrapping
@@ -890,10 +893,6 @@
return KeepGoing;
}
-bool allowInvalidBaseExpr() const {
- return EvalMode == EM_OffsetFold;
-}
-
class ArrayInitLoopIndex {
EvalInfo
uint64_t OuterIndex;
@@ -1394,8 +1393,10 @@
static bool
