Relax constexpr rules to improve __builtin_object_size's accuracy

[george.burgess.iv]

Summary

(Hoping the formatting works as one would expect)

Motivating examples:
Pre-patch:

__builtin_object_size((char*)&foo, 0) != __builtin_object_size(&foo, 0) // if __builtin_object_size(&foo, 0) != -1
__builtin_object_size(&foo[1].bar[-1].baz, 1) == -1. // Always.

Post-patch:
Both act as one would expect. This was accomplished by making three changes:

• Adding a flag to PointerExprEvaluator that makes it more accepting of reinterpret_casts.
• Making array index/pointer offset less coupled in PointerExprEvaluator (we now carry around an extra Offset field that denotes how far we are away from an object boundary).
• Adding an OutermostMemberEvaluator that ignores foo[1].bar[-1] in foo[1].bar[-1].baz, and is more relaxed with casts/pointer arithmetic/etc. (Not 100% sold on the name)

[vsk]

Just a minor nitpick.

lib/AST/ExprConstant.cpp

51

Do you need <iostream>?

[george.burgess.iv]

Removed advanced debugging tools (iostream)

lib/AST/ExprConstant.cpp

51

Artifact of print debugging. Thanks for catching that :)

[rsmith]

lib/AST/ExprConstant.cpp

4763

This should be handled as an EvaluationMode.

4879–4907

We should not do this except in your special new mode, and I'm not entirely convinced we should support this case at all. Just discarding the designator (as we would do anyway in constant-folding mode) seems like an acceptable response here. We really don't know what subobject is being referenced any more.

6355–6356

I think handling this through the normal pointer evaluator, but with a different EvaluationMode, is a better approach. (We need some representation for an LValueBase to represent "unknown base", but everything else should fall through pretty naturally.)

[george.burgess.iv]

lib/AST/ExprConstant.cpp

4763

Works for me.

4879–4907

We need some way to support BOS((char*)&Foo.Bar + 1, N), so this needs to go somewhere. I originally had it in its own method that was super similar to this one, and the duplication made me sad. Will re-duplicate and see how that looks.

FYI, if you'll look at the diff at L194 in p2-0x.cpp, we have an interesting feature in the non-patched impl where B *b = (B*)((A*)&b + 1) is at an offset of sizeof(A), but Designator says it's at b[1]. I assumed this behavior is a bug, and this kinda-fixes it. If it's intended behavior, I'll absolutely find a better way to handle this.

6355–6356

Sounds great to me -- I thought another visitor seemed heavyweight, but didn't consider adding another EvaluationMode. Will look into swapping approaches.

[george.burgess.iv]

Mostly redid the patch; removed OutermostExprEvaluator, moved evaluation intent into EvaluationMode, made it so the more interesting rulebreaking that __builtin_object_size allows is only allowed when evaluating __builtin_object_size.

Not entirely sold on adding 4 EvaluationModes, but we need to know two bits of information, both of which impact how we evaluate things:

• Whether we're evaluating an actual ConstantExpression or a PotentialConstantExpression (if we always assume the former, we get memory corruption by reaching into CallStackFrame::Arguments sometimes; if we assume the latter, attribute(enable_if) breaks).
• If we're allowed to assume that the base of a MemberExpr is valid.

Also, I'm not 100% on calling LValue.set(MaybeAnRValue) if we are assuming that the base of a MemberExpr is valid. However, in such a case, I would think that it's safe to assume that the caller doesn't care about the base. Plus, it's In The Documentation(TM) that you shouldn't rely on Result.getLValueBase() being a valid base when the EvalMode would allow this assumption to be made.

[george.burgess.iv]

Talked with Richard, and we both agree that adding 4 EvaluationModes is too much for the use case. So, we decided to add a flag to LValue to denote that the LValueBase is invalid. This allows us to get by with just 2 new EvaluationModes, which is much more acceptable.

LValueBases can only be invalid if you're using one of the shiny new EvaluationModes.

[rsmith]

lib/AST/ExprConstant.cpp

499

constexpr is a keyword and an adjective. The noun phrase you want here is "constant expressions".

504–507

I think you should only have one mode here, and it should not be a "checking potential constant expression" mode. (Then, if evaluation of a __builtin_object_size call fails from a "checking potential constant expression" mode, we should treat it as a potential success -- that is, return false with no diagnostic.)

It looks like the only place this would go wrong is the assertion in CheckLValueConstantExpression, but you can update that assertion to also allow this one case.

851

I'm not convinced this is a good idea. There are two cases:

1. We're looking for the complete object (type is 0 or 2). We don't need this change: the right thing to do is discard the subobject designator and just track the offset, which is what we'd do anyway and all we need.

2. We're looking for the specified subobject (type is 1 or 3). This change seems wrong: we no longer know which the specified subobject is, because the code computed an offset that left the subobject we thought we were in.

1945–1948

Same comment as for the previous case: I don't think this is a good idea. We can track the offset in this case, but we can't reasonably keep the designator valid.

3959–3964

This doesn't make much sense in ExprEvaluatorBase. I think you should only need the change to LValueExprEvaluatorBase::VisitMemberExpr -- it's only lvalue member expressions that should get this behavior.

4654–4660

Why do you hate comments? :)

4818–4846

We need some way to support BOS((char*)&Foo.Bar + 1, N)

Why? This seems like a pretty canonical example of "we do not know which subobject is being referenced any more", and thus we should give up for types 1 and 3 here.

4818–4846

[...] we have an interesting feature in the non-patched impl where B *b = (B*)((A*)&b + 1) is at an offset of sizeof(A), but Designator says it's at b[1]

That certainly seems like a bug. We should have marked the designator as invalid at the pointer arithmetic because it wasn't a pointer to the most derived type. (Are you sure it wasn't your changes to adjustIndex that caused this?)

4913–4920

You need to set the designator to invalid here, or you break the invariant of the LValue and further calculations on it will result in wrong offsets (or maybe assertions). As with the other two cases, I don't think it's reasonable to claim we can track the subobject once this has happened.

6239

Lowercase first letter for function names is preferred in new code.

6288–6306

Not really related to the current patch, but this check seems wrong for the (Type & 1) == 0 case: if the designator is invalid, we don't care what MostDerivedType is, we only care about the type of the Base and the BaseOffset (and now, having a valid base).

6358

We're supposed to return the number of usable bytes; I don't see why we should care if we can't fit an integral number of the pointee type in those bytes.

[george.burgess.iv]

Addressed all feedback; backed out support for more questionable features (negative indices, non-object-boundary offsets, etc), added logic to ignore pointless casts, and added tests.

We're down to one new EvalMode (which is only used on Type=1|3).

[rsmith]

This essentially looks fine. Let me know if you want me to take another look once you've fixed the ignorePointerCastsAndParens bug with derived-to-base conversions, otherwise go ahead.

lib/AST/ExprConstant.cpp

499

Maybe rename to OffsetFold or DesignatorFold?

6263–6264

I don't think this is quite right: you should only skip past casts that don't change the pointer value. In particular, this check will step past derived-to-base pointer conversions, which may require an adjustment to the pointer.

[george.burgess.iv]

r246877. Thanks for the review!

(Also: Forgot to submit comments with the most recent revision, so you're getting them all now. Sorry. :) )

lib/AST/ExprConstant.cpp

499

DesignatorFold it is -- thanks for the suggestions.

504–507

Migrated to EM_ConstantFold (if Type=0|2) or EM_ConstantExpressionConstantFold (if Type=1|3)

851

SGTM. Backed it out.

4654–4660

They were far too concise and up-to-date for my taste.

4818–4846

Why? This seems like a pretty canonical example of "we do not know which subobject is being referenced any more", and thus we should give up for types 1 and 3 here.

SGTM.

That certainly seems like a bug...

Discussed in person. Was a misunderstanding on my part.

6239

Thanks for the heads-up :)

6263–6264

Thanks for catching that -- I really need to get to know C++ better :)

Updated if (Cast == nullptr) to if (Cast == nullptr || Cast->getCastKind() == CK_DerivedToBase).

6288–6306

We now no longer directly deal with this case, so I'm closing this.

6358

Note: Code is gone, so this no longer applies. Original response below

The only reason we care is because of how we compute the end offset. Currently, we do EndOffset = SizeOfPointee * NumberOfArraySlotsUntilTheEnd. This works fine when we're on an object boundary, but when we're not, the array index = floor(OffsetFromArrayStart / SizeOfPointee). So, without this line, EndOffset = 11 in:

int16_t shorts[5];
__builtin_object_size((char*)&shorts + 1, 1);

I'm happy to phrase it differently in the code, but if we decide to allow off-object-boundary offsets, then we need to find some way to support it.

[steven_wu]

This commit seems to cause miscompile in LNT testsuite with -O0 and -O3
http://lab.llvm.org:8080/green/job/perf_o0g_run/7070/warnings2Result/new/
http://lab.llvm.org:8080/green/job/perf_o3lto_run/15591/warnings2Result/new/

Looks like there is an undefined in LNT testsuite, here is the relevant code for consumer-lame:

typedef struct                                                                  
{                                                                               
  int used;                                                                     
  int valid;                                                                    
  char title[31];                                                               
  char artist[31];                                                              
  char album[31];                                                               
  char year[5];                                                                 
  char comment[31];                                                             
  char tagtext[128];                                                            
  char genre[1];                                                                
  unsigned char track;                                                          
                                                                                
}   ID3TAGDATA; 
void id3_inittag(ID3TAGDATA *tag) {                                             
  ...
  strcpy( tag->genre, "ÿ"); /* unset genre */      
  ...                             
}

Here is the suggested change:

diff --git a/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c b/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c
index e24a966..23f2b86 100644
--- a/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c
+++ b/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c
@@ -34,7 +34,7 @@ void id3_inittag(ID3TAGDATA *tag) {
        strcpy( tag->album, "");
        strcpy( tag->year, "");    
        strcpy( tag->comment, "");
-       strcpy( tag->genre, "<FF>");    /* unset genre */
+       tag->genre[0] = '<FF>'; /* unset genre */
        tag->track = 0;
 
        tag->valid = 0;         /* not ready for writing*/

However, I have trouble understand the code in consumer-typeset. It crashes in z07.c:138
By the looks of it, the code is trying to copy the include path (specified on command line), which is setup to be "$PATH_TO_TEST_SUITE/MultiSource/Benchmarks/MiBench/consumer-typeset/data/include" into unsigned char ostring[4]. From the look of the code, it is trying to utilize all the size in OBJECT to store the entire string. I don't know if that is fixable. Let me know if you have any better idea of how to fix that.

lib/AST/ExprConstant.cpp

 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/Builtins.h"
 #include "clang/Basic/TargetInfo.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstring>
 #include <functional>
 
 using namespace clang;
 using llvm::APSInt;
 using llvm::APFloat;
 
 static bool IsGlobalLValue(APValue::LValueBase B);
 
 namespace {
   struct LValue;
   struct CallStackFrame;
enum EvaluationMode {
 
       /// Evaluate as a potential constant expression. Keep going if we hit a
       /// construct that we can't evaluate yet (because we don't yet know the
       /// value of something) but stop if we hit something that could never be
       /// a constant expression. Some expressions can be retried in the
       /// optimizer if we don't constant fold them here, but in an unevaluated
       /// context we try to fold them immediately since the optimizer never
       /// gets a chance to look at it.
-      EM_PotentialConstantExpressionUnevaluated
+      EM_PotentialConstantExpressionUnevaluated,
+
+      /// Evaluate as a constant expression. Continue evaluating if we find a
+      /// MemberExpr with a base that can't be evaluated.
+      EM_ConstantExpressionOffsetFold,
     } EvalMode;
 
     /// Are we checking whether the expression is a potential constant
     /// expression?
     bool checkingPotentialConstantExpression() const {
       return EvalMode == EM_PotentialConstantExpression ||
              EvalMode == EM_PotentialConstantExpressionUnevaluated;
     }
 
     /// Are we checking an expression for overflow?
     // FIXME: We should check for any kind of undefined or suspicious behavior
     // in such constructs, not just overflow.
     bool checkingForOverflow() { return EvalMode == EM_EvaluateForOverflow; }
 
     EvalInfo(const ASTContext &C, Expr::EvalStatus &S, EvaluationMode Mode)
       : Ctx(const_cast<ASTContext &>(C)), EvalStatus(S), CurrentCall(nullptr),
OptionalDiagnostic Diag(SourceLocation Loc, diag::kind DiagId
             if (!EvalStatus.HasSideEffects)
               break;
             // We've had side-effects; we want the diagnostic from them, not
             // some later problem.
           case EM_ConstantExpression:
           case EM_PotentialConstantExpression:
           case EM_ConstantExpressionUnevaluated:
           case EM_PotentialConstantExpressionUnevaluated:
+          case EM_ConstantExpressionOffsetFold:
             HasActiveDiagnostic = false;
             return OptionalDiagnostic();
           }
         }
 
         unsigned CallStackNotes = CallStackDepth - 1;
         unsigned Limit = Ctx.getDiagnostics().getConstexprBacktraceLimit();
         if (Limit)
bool keepEvaluatingAfterSideEffect() {
       case EM_PotentialConstantExpressionUnevaluated:
       case EM_EvaluateForOverflow:
       case EM_IgnoreSideEffects:
         return true;
 
       case EM_ConstantExpression:
       case EM_ConstantExpressionUnevaluated:
       case EM_ConstantFold:
+      case EM_ConstantExpressionOffsetFold:
         return false;
       }
       llvm_unreachable("Missed EvalMode case");
     }
 
     /// Note that we have had a side-effect, and determine whether we should
     /// keep evaluating.
     bool noteSideEffect() {
bool keepEvaluatingAfterFailure() {
       case EM_PotentialConstantExpressionUnevaluated:
       case EM_EvaluateForOverflow:
         return true;
 
       case EM_ConstantExpression:
       case EM_ConstantExpressionUnevaluated:
       case EM_ConstantFold:
       case EM_IgnoreSideEffects:
+      case EM_ConstantExpressionOffsetFold:
         return false;
       }
       llvm_unreachable("Missed EvalMode case");
     }
+
+    bool allowInvalidBaseExpr() const {
+      return EvalMode == EM_ConstantExpressionOffsetFold;
+    }
   };
 
   /// Object used to treat all foldable expressions as constant expressions.
   struct FoldConstant {
     EvalInfo &Info;
     bool Enabled;
     bool HadNoPriorDiags;
     EvalInfo::EvaluationMode OldMode;
struct FoldConstant {
     ~FoldConstant() {
       if (Enabled && HadNoPriorDiags && !Info.EvalStatus.Diag->empty() &&
           !Info.EvalStatus.HasSideEffects)
         Info.EvalStatus.Diag->clear();
       Info.EvalMode = OldMode;
     }
   };
 
+  /// RAII object used to treat the current evaluation as the correct pointer
+  /// offset fold for the current EvalMode
+  struct FoldOffsetRAII {
+    EvalInfo &Info;
+    EvalInfo::EvaluationMode OldMode;
+    explicit FoldOffsetRAII(EvalInfo &Info, bool Subobject)
+        : Info(Info), OldMode(Info.EvalMode) {
+      if (!Info.checkingPotentialConstantExpression())
+        Info.EvalMode = Subobject ? EvalInfo::EM_ConstantExpressionOffsetFold
+                                  : EvalInfo::EM_ConstantFold;
+    }
+
+    ~FoldOffsetRAII() { Info.EvalMode = OldMode; }
+  };
+
   /// RAII object used to suppress diagnostics and side-effects from a
   /// speculative evaluation.
   class SpeculativeEvaluationRAII {
     EvalInfo &Info;
     Expr::EvalStatus Old;
 
   public:
     SpeculativeEvaluationRAII(EvalInfo &Info,
Info.CCEDiag(E, diag::note_constexpr_array_index)
       << static_cast<int>(N) << /*non-array*/ 1;
   setInvalid();
 }
 
 CallStackFrame::CallStackFrame(EvalInfo &Info, SourceLocation CallLoc,
                                const FunctionDecl *Callee, const LValue *This,
                                APValue *Arguments)
     : Info(Info), Caller(Info.CurrentCall), CallLoc(CallLoc), Callee(Callee),
       Index(Info.NextCallIndex++), This(This), Arguments(Arguments) {
   Info.CurrentCall = this;
   ++Info.CallStackDepth;
 }
 
 CallStackFrame::~CallStackFrame() {
   assert(Info.CurrentCall == this && "calls retired out of order");
   --Info.CallStackDepth;
   Info.CurrentCall = Caller;
void setFrom(const APValue &v) {
         IntImag = v.getComplexIntImag();
       }
     }
   };
 
   struct LValue {
     APValue::LValueBase Base;
     CharUnits Offset;
-    unsigned CallIndex;
+    bool InvalidBase : 1;
+    unsigned CallIndex : 31;
     SubobjectDesignator Designator;
 
     const APValue::LValueBase getLValueBase() const { return Base; }
     CharUnits &getLValueOffset() { return Offset; }
     const CharUnits &getLValueOffset() const { return Offset; }
     unsigned getLValueCallIndex() const { return CallIndex; }
     SubobjectDesignator &getLValueDesignator() { return Designator; }
     const SubobjectDesignator &getLValueDesignator() const { return Designator;}
 
     void moveInto(APValue &V) const {
       if (Designator.Invalid)
         V = APValue(Base, Offset, APValue::NoLValuePath(), CallIndex);
       else
         V = APValue(Base, Offset, Designator.Entries,
                     Designator.IsOnePastTheEnd, CallIndex);
     }
     void setFrom(ASTContext &Ctx, const APValue &V) {
       assert(V.isLValue());
       Base = V.getLValueBase();
       Offset = V.getLValueOffset();
+      InvalidBase = false;
       CallIndex = V.getLValueCallIndex();
       Designator = SubobjectDesignator(Ctx, V);
     }
 
-    void set(APValue::LValueBase B, unsigned I = 0) {
+    void set(APValue::LValueBase B, unsigned I = 0, bool BInvalid = false) {
       Base = B;
       Offset = CharUnits::Zero();
+      InvalidBase = BInvalid;
       CallIndex = I;
       Designator = SubobjectDesignator(getType(B));
     }
 
+    void setInvalid(APValue::LValueBase B, unsigned I = 0) {
+      set(B, I, true);
+    }
+
     // Check that this LValue is not based on a null pointer. If it is, produce
     // a diagnostic and mark the designator as invalid.
     bool checkNullPointer(EvalInfo &Info, const Expr *E,
                           CheckSubobjectKind CSK) {
       if (Designator.Invalid)
         return false;
       if (!Base) {
         Info.CCEDiag(E, diag::note_constexpr_null_subobject)
static bool HandleLValueArrayAdjustment(EvalInfo &Info, const Expr *E,
   if (!HandleSizeof(Info, E->getExprLoc(), EltTy, SizeOfPointee))
     return false;
 
   // Compute the new offset in the appropriate width.
   LVal.Offset += Adjustment * SizeOfPointee;
   LVal.adjustIndex(Info, E, Adjustment);
   return true;
 }
 
 /// Update an lvalue to refer to a component of a complex number.
 /// \param Info - Information about the ongoing evaluation.
 /// \param LVal - The lvalue to be updated.
 /// \param EltTy - The complex number's component type.
 /// \param Imag - False for the real component, true for the imaginary.
 static bool HandleLValueComplexElement(EvalInfo &Info, const Expr *E,
                                        LValue &LVal, QualType EltTy,
                                        bool Imag) {
   if (Imag) {
     CharUnits SizeOfComponent;
     if (!HandleSizeof(Info, E->getExprLoc(), EltTy, SizeOfComponent))

 private:
   Derived &getDerived() { return static_cast<Derived&>(*this); }
   bool DerivedSuccess(const APValue &V, const Expr *E) {
     return getDerived().Success(V, E);
   }
   bool DerivedZeroInitialization(const Expr *E) {
     return getDerived().ZeroInitialization(E);
   }
 
   // Check whether a conditional operator with a non-constant condition is a
   // potential constant expression. If neither arm is a potential constant
   // expression, then the conditional operator is not either.
   template<typename ConditionalOperator>
   void CheckPotentialConstantConditional(const ConditionalOperator *E) {
     assert(Info.checkingPotentialConstantExpression());
 
     // Speculatively evaluate both arms.
     {
       SmallVector<PartialDiagnosticAt, 8> Diag;
       SpeculativeEvaluationRAII Speculate(Info, &Diag);
 
       StmtVisitorTy::Visit(E->getFalseExpr());
public:
   bool Success(const APValue &V, const Expr *E) {
     Result.setFrom(this->Info.Ctx, V);
     return true;
   }
 
   bool VisitMemberExpr(const MemberExpr *E) {
     // Handle non-static data members.
     QualType BaseTy;
+    bool EvalOK;
     if (E->isArrow()) {
-      if (!EvaluatePointer(E->getBase(), Result, this->Info))
-        return false;
+      EvalOK = EvaluatePointer(E->getBase(), Result, this->Info);
       BaseTy = E->getBase()->getType()->castAs<PointerType>()->getPointeeType();
     } else if (E->getBase()->isRValue()) {
       assert(E->getBase()->getType()->isRecordType());
-      if (!EvaluateTemporary(E->getBase(), Result, this->Info))
-        return false;
+      EvalOK = EvaluateTemporary(E->getBase(), Result, this->Info);
       BaseTy = E->getBase()->getType();
     } else {
-      if (!this->Visit(E->getBase()))
-        return false;
+      EvalOK = this->Visit(E->getBase());
       BaseTy = E->getBase()->getType();
     }
+    if (!EvalOK) {
+      if (!this->Info.allowInvalidBaseExpr())
+        return false;
+      Result.setInvalid(E->getBase());
+    }
 
     const ValueDecl *MD = E->getMemberDecl();
     if (const FieldDecl *FD = dyn_cast<FieldDecl>(E->getMemberDecl())) {
       assert(BaseTy->getAs<RecordType>()->getDecl()->getCanonicalDecl() ==
              FD->getParent()->getCanonicalDecl() && "record / field mismatch");
       (void)BaseTy;
       if (!HandleLValueMember(this->Info, E, Result, FD))
         return false;
bool LValueExprEvaluator::VisitCXXTypeidExpr(const CXXTypeidExpr *E) {
   return false;
 }
 
 bool LValueExprEvaluator::VisitCXXUuidofExpr(const CXXUuidofExpr *E) {
   return Success(E);
 }
 
 bool LValueExprEvaluator::VisitMemberExpr(const MemberExpr *E) {
   // Handle static data members.
   if (const VarDecl *VD = dyn_cast<VarDecl>(E->getMemberDecl())) {
     VisitIgnoredValue(E->getBase());
     return VisitVarDecl(E, VD);
   }
 
   // Handle static member functions.
   if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(E->getMemberDecl())) {
     if (MD->isStatic()) {
       VisitIgnoredValue(E->getBase());
       return Success(MD);
     }
   }
 
   // Handle non-static data members.
return handleIncDec(
       UO->isIncrementOp(), nullptr);
 }
 
 bool LValueExprEvaluator::VisitCompoundAssignOperator(
     const CompoundAssignOperator *CAO) {
   if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())
     return Error(CAO);
 
   APValue RHS;
 
   // The overall lvalue result is the result of evaluating the LHS.
   if (!this->Visit(CAO->getLHS())) {
     if (Info.keepEvaluatingAfterFailure())
       Evaluate(RHS, this->Info, CAO->getRHS());
     return false;
   }
 
bool Success(const Expr *E) {
     Result.set(E);
     return true;
   }
 public:
 
   PointerExprEvaluator(EvalInfo &info, LValue &Result)
     : ExprEvaluatorBaseTy(info), Result(Result) {}
 
   bool Success(const APValue &V, const Expr *E) {
     Result.setFrom(Info.Ctx, V);
     return true;
   }
   bool ZeroInitialization(const Expr *E) {
     return Success((Expr*)nullptr);
   }
 
   bool VisitBinaryOperator(const BinaryOperator *E);
   bool VisitCastExpr(const CastExpr* E);
   bool VisitUnaryAddrOf(const UnaryOperator *E);
   bool VisitObjCStringLiteral(const ObjCStringLiteral *E)
       { return Success(E); }
   bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E)
-      { return Success(E); }    
+      { return Success(E); }
   bool VisitAddrLabelExpr(const AddrLabelExpr *E)
       { return Success(E); }
   bool VisitCallExpr(const CallExpr *E);
   bool VisitBlockExpr(const BlockExpr *E) {
     if (!E->getBlockDecl()->hasCaptures())
       return Success(E);
     return Error(E);
   }
   bool VisitCXXThisExpr(const CXXThisExpr *E) {
     // Can't look at 'this' when checking a potential constant expression.
     if (Info.checkingPotentialConstantExpression())
       return false;
     if (!Info.CurrentCall->This) {
       if (Info.getLangOpts().CPlusPlus11)
         Info.Diag(E, diag::note_constexpr_this) << E->isImplicit();
       else
         Info.Diag(E);
       return false;
     }
     Result = *Info.CurrentCall->This;
     return true;
   }
bool PointerExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
   const Expr *IExp = E->getRHS();
   if (IExp->getType()->isPointerType())
     std::swap(PExp, IExp);
 
   bool EvalPtrOK = EvaluatePointer(PExp, Result, Info);
   if (!EvalPtrOK && !Info.keepEvaluatingAfterFailure())
     return false;
 
   llvm::APSInt Offset;
   if (!EvaluateInteger(IExp, Offset, Info) || !EvalPtrOK)
     return false;
 
   int64_t AdditionalOffset = getExtValue(Offset);
   if (E->getOpcode() == BO_Sub)
     AdditionalOffset = -AdditionalOffset;
 
   QualType Pointee = PExp->getType()->castAs<PointerType>()->getPointeeType();
   return HandleLValueArrayAdjustment(Info, E, Result, Pointee,
                                      AdditionalOffset);
 }
 
 bool PointerExprEvaluator::VisitUnaryAddrOf(const UnaryOperator *E) {
   return EvaluateLValue(E->getSubExpr(), Result, Info);
 }
 
 bool PointerExprEvaluator::VisitCastExpr(const CastExpr* E) {
   const Expr* SubExpr = E->getSubExpr();
 
   switch (E->getCastKind()) {
   default:
     break;
 
   case CK_BitCast:
   case CK_CPointerToObjCPointerCast:
   case CK_BlockPointerToObjCPointerCast:
   case CK_AnyPointerToBlockPointerCast:
   case CK_AddressSpaceConversion:
     if (!Visit(SubExpr))
       return false;
     // Bitcasts to cv void* are static_casts, not reinterpret_casts, so are
     // permitted in constant expressions in C++11. Bitcasts from cv void* are
     // also static_casts, but we disallow them as a resolution to DR1312.
     if (!E->getType()->isVoidPointerType()) {
       Result.Designator.setInvalid();
       if (SubExpr->getType()->isVoidPointerType())
         CCEDiag(E, diag::note_constexpr_invalid_cast)
           << 3 << SubExpr->getType();
       else
         CCEDiag(E, diag::note_constexpr_invalid_cast) << 2;
     }
     return true;
 
   case CK_DerivedToBase:
   case CK_UncheckedDerivedToBase:
     if (!EvaluatePointer(E->getSubExpr(), Result, Info))
       return false;
     if (!Result.Base && Result.Offset.isZero())
       return true;
case CK_IntegralToPointer: {
     APValue Value;
     if (!EvaluateIntegerOrLValue(SubExpr, Value, Info))
       break;
 
     if (Value.isInt()) {
       unsigned Size = Info.Ctx.getTypeSize(E->getType());
       uint64_t N = Value.getInt().extOrTrunc(Size).getZExtValue();
       Result.Base = (Expr*)nullptr;
+      Result.InvalidBase = false;
       Result.Offset = CharUnits::fromQuantity(N);
       Result.CallIndex = 0;
       Result.Designator.setInvalid();
       return true;
     } else {
       // Cast is of an lvalue, no need to change value.
       Result.setFrom(Info.Ctx, Value);
       return true;
static bool EvaluateBuiltinConstantP(ASTContext &Ctx, const Expr *Arg) {
   }
 
   // Anything else isn't considered to be sufficiently constant.
   return false;
 }
 
 /// Retrieves the "underlying object type" of the given expression,
 /// as used by __builtin_object_size.
 static QualType getObjectType(APValue::LValueBase B) {
   if (const ValueDecl *D = B.dyn_cast<const ValueDecl*>()) {
     if (const VarDecl *VD = dyn_cast<VarDecl>(D))
       return VD->getType();
   } else if (const Expr *E = B.get<const Expr*>()) {
     if (isa<CompoundLiteralExpr>(E))
       return E->getType();
   }
 
   return QualType();
 }
 
+/// A more selective version of E->IgnoreParenCasts for
+/// TryEvaluateBuiltinObjectSize. Will not ignore casts that make E an rvalue
+/// or pointer from a non-rvalue or non-pointer, respectively.
+static const Expr *ignorePointerCastsAndParens(const Expr *E) {
+  assert(E->isRValue() && E->getType()->hasPointerRepresentation());
+
+  auto *NoParens = E->IgnoreParens();
+  auto *Cast = dyn_cast<CastExpr>(NoParens);
+  if (Cast == nullptr)
+    return NoParens;
+
+  auto *SubExpr = Cast->getSubExpr();
+  if (!SubExpr->getType()->hasPointerRepresentation() || !SubExpr->isRValue())
+    return NoParens;
+  return ignorePointerCastsAndParens(SubExpr);
+}
+
 bool IntExprEvaluator::TryEvaluateBuiltinObjectSize(const CallExpr *E,
                                                     unsigned Type) {
   // Determine the denoted object.
   LValue Base;
   {
     // The operand of __builtin_object_size is never evaluated for side-effects.
     // If there are any, but we can determine the pointed-to object anyway, then
     // ignore the side-effects.
     SpeculativeEvaluationRAII SpeculativeEval(Info);
-    FoldConstant Fold(Info, true);
-    if (!EvaluatePointer(E->getArg(0), Base, Info))
+    FoldOffsetRAII Fold(Info, Type & 1);
+    const Expr *Ptr = ignorePointerCastsAndParens(E->getArg(0));
+    if (!EvaluatePointer(Ptr, Base, Info))
       return false;
   }
 
   CharUnits BaseOffset = Base.getLValueOffset();
-
-  // If we point to before the start of the object, there are no
-  // accessible bytes.
-  if (BaseOffset < CharUnits::Zero())
+  // If we point to before the start of the object, there are no accessible
+  // bytes.
+  if (BaseOffset.isNegative())
     return Success(0, E);
 
-  // MostDerivedType is null if we're dealing with a literal such as nullptr or
-  // (char*)0x100000. Lower it to LLVM in either case so it can figure out what
-  // to do with it.
-  // FIXME(gbiv): Try to do a better job with this in clang.
-  if (Base.Designator.MostDerivedType.isNull())
+  // In the case where we're not dealing with a subobject, we discard the
+  // subobject bit.
+  if (!Base.Designator.Invalid && Base.Designator.Entries.empty())
+    Type = Type & ~1U;
+
+  // If Type & 1 is 0, we need to be able to statically guarantee that the bytes
+  // exist. If we can't verify the base, then we can't do that.
+  //
+  // As a special case, we produce a valid object size for an unknown object
+  // with a known designator if Type & 1 is 1. For instance:
+  //
+  //   extern struct X { char buff[32]; int a, b, c; } *p;
+  //   int a = __builtin_object_size(p->buff + 4, 3); // returns 28
+  //   int b = __builtin_object_size(p->buff + 4, 2); // returns 0, not 40
+  //
+  // This matches GCC's behavior.
+  if ((Type & 1) == 0 && Base.InvalidBase)
     return Error(E);
 
   // If Type & 1 is 0, the object in question is the complete object; reset to
   // a complete object designator in that case.
   //
   // If Type is 1 and we've lost track of the subobject, just find the complete
   // object instead. (If Type is 3, that's not correct behavior and we should
   // return 0 instead.)
   LValue End = Base;
   if (((Type & 1) == 0) || (End.Designator.Invalid && Type == 1)) {
     QualType T = getObjectType(End.getLValueBase());
     if (T.isNull())
       End.Designator.setInvalid();
     else {
       End.Designator = SubobjectDesignator(T);
       End.Offset = CharUnits::Zero();
     }
   }
 
-  // FIXME: We should produce a valid object size for an unknown object with a
-  // known designator, if Type & 1 is 1. For instance:
-  //
-  //   extern struct X { char buff[32]; int a, b, c; } *p;
-  //   int a = __builtin_object_size(p->buff + 4, 3); // returns 28
-  //   int b = __builtin_object_size(p->buff + 4, 2); // returns 0, not 40
-  //
-  // This is GCC's behavior. We currently don't do this, but (hopefully) will in
-  // the near future.
-
   // If it is not possible to determine which objects ptr points to at compile
   // time, __builtin_object_size should return (size_t) -1 for type 0 or 1
   // and (size_t) 0 for type 2 or 3.
   if (End.Designator.Invalid)
     return false;
 
   // According to the GCC documentation, we want the size of the subobject
   // denoted by the pointer. But that's not quite right -- what we actually
   // want is the size of the immediately-enclosing array, if there is one.
   int64_t AmountToAdd = 1;
   if (End.Designator.MostDerivedArraySize &&
       End.Designator.Entries.size() == End.Designator.MostDerivedPathLength) {
     // We got a pointer to an array. Step to its end.
     AmountToAdd = End.Designator.MostDerivedArraySize -
-                  End.Designator.Entries.back().ArrayIndex;
-  } else if (End.Designator.IsOnePastTheEnd) {
+  } else if (End.Designator.isOnePastTheEnd()) {
     // We're already pointing at the end of the object.
     AmountToAdd = 0;
   }
 
-  if (End.Designator.MostDerivedType->isIncompleteType() ||
-      End.Designator.MostDerivedType->isFunctionType())
+  QualType PointeeType = End.Designator.MostDerivedType;
+  assert(!PointeeType.isNull());
+  if (PointeeType->isIncompleteType() || PointeeType->isFunctionType())
     return Error(E);
 
   if (!HandleLValueArrayAdjustment(Info, E, End, End.Designator.MostDerivedType,
                                    AmountToAdd))
     return false;
 
   auto EndOffset = End.getLValueOffset();
   if (BaseOffset > EndOffset)
     return Success(0, E);
 
   return Success(EndOffset - BaseOffset, E);
 }
 
 bool IntExprEvaluator::VisitCallExpr(const CallExpr *E) {
   switch (unsigned BuiltinOp = E->getBuiltinCallee()) {
   default:
     return ExprEvaluatorBaseTy::VisitCallExpr(E);
 
   case Builtin::BI__builtin_object_size: {
case Builtin::BI__builtin_object_size: {
     // Expression had no side effects, but we couldn't statically determine the
     // size of the referenced object.
     switch (Info.EvalMode) {
     case EvalInfo::EM_ConstantExpression:
     case EvalInfo::EM_PotentialConstantExpression:
     case EvalInfo::EM_ConstantFold:
     case EvalInfo::EM_EvaluateForOverflow:
     case EvalInfo::EM_IgnoreSideEffects:
+    case EvalInfo::EM_ConstantExpressionOffsetFold:
       // Leave it to IR generation.
       return Error(E);
     case EvalInfo::EM_ConstantExpressionUnevaluated:
     case EvalInfo::EM_PotentialConstantExpressionUnevaluated:
       // Reduce it to a constant now.
       return Success((Type & 2) ? 0 : -1, E);
     }
   }

test/CodeGen/object-size.c

void test19() {
   // CHECK: store i32 8
   gi = __builtin_object_size(&foo.a, 0);
   // CHECK: store i32 4
   gi = __builtin_object_size(&foo.a, 1);
   // CHECK: store i32 8
   gi = __builtin_object_size(&foo.a, 2);
   // CHECK: store i32 4
   gi = __builtin_object_size(&foo.a, 3);
+
+  // CHECK: store i32 4
+  gi = __builtin_object_size(&foo.b, 0);
+  // CHECK: store i32 4
+  gi = __builtin_object_size(&foo.b, 1);
+  // CHECK: store i32 4
+  gi = __builtin_object_size(&foo.b, 2);
+  // CHECK: store i32 4
+  gi = __builtin_object_size(&foo.b, 3);
 }
 
 // CHECK: @test20
 void test20() {
   struct { int t[10]; } t[10];
 
   // CHECK: store i32 380
   gi = __builtin_object_size(&t[0].t[5], 0);
void test22() {
   // CHECK: store i32 0
   gi = __builtin_object_size(&t[9].t[10], 0);
   // CHECK: store i32 0
   gi = __builtin_object_size(&t[9].t[10], 1);
   // CHECK: store i32 0
   gi = __builtin_object_size(&t[9].t[10], 2);
   // CHECK: store i32 0
   gi = __builtin_object_size(&t[9].t[10], 3);
+
+  // CHECK: store i32 0
+  gi = __builtin_object_size((char*)&t[0] + sizeof(t), 0);
+  // CHECK: store i32 0
+  gi = __builtin_object_size((char*)&t[0] + sizeof(t), 1);
+  // CHECK: store i32 0
+  gi = __builtin_object_size((char*)&t[0] + sizeof(t), 2);
+  // CHECK: store i32 0
+  gi = __builtin_object_size((char*)&t[0] + sizeof(t), 3);
+
+  // CHECK: store i32 0
+  gi = __builtin_object_size((char*)&t[9].t[0] + 10*sizeof(t[0].t), 0);
+  // CHECK: store i32 0
+  gi = __builtin_object_size((char*)&t[9].t[0] + 10*sizeof(t[0].t), 1);
+  // CHECK: store i32 0
+  gi = __builtin_object_size((char*)&t[9].t[0] + 10*sizeof(t[0].t), 2);
+  // CHECK: store i32 0
+  gi = __builtin_object_size((char*)&t[9].t[0] + 10*sizeof(t[0].t), 3);
 }
 
-struct Test23Ty { int t[10]; };
+struct Test23Ty { int a; int t[10]; };
 
 // CHECK: @test23
-void test23(struct Test22Ty *p) {
+void test23(struct Test23Ty *p) {
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
   gi = __builtin_object_size(p, 0);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
   gi = __builtin_object_size(p, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true)
   gi = __builtin_object_size(p, 2);
-
   // Note: this is currently fixed at 0 because LLVM doesn't have sufficient
   // data to correctly handle type=3
   // CHECK: store i32 0
   gi = __builtin_object_size(p, 3);
-}
 
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
+  gi = __builtin_object_size(&p->a, 0);
+  // CHECK: store i32 4
+  gi = __builtin_object_size(&p->a, 1);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true)
+  gi = __builtin_object_size(&p->a, 2);
+  // CHECK: store i32 4
+  gi = __builtin_object_size(&p->a, 3);
+
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
+  gi = __builtin_object_size(&p->t[5], 0);
+  // CHECK: store i32 20
+  gi = __builtin_object_size(&p->t[5], 1);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true)
+  gi = __builtin_object_size(&p->t[5], 2);
+  // CHECK: store i32 20
+  gi = __builtin_object_size(&p->t[5], 3);
+}
 
 // PR24493 -- ICE if __builtin_object_size called with NULL and (Type & 1) != 0
 // CHECK: @test24
 void test24() {
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false)
   gi = __builtin_object_size((void*)0, 0);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false)
   gi = __builtin_object_size((void*)0, 1);
void test25() {
   gi = __builtin_object_size((void*)0 + 0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true)
   gi = __builtin_object_size((void*)0 + 0x1000, 2);
   // Note: Currently fixed at zero because LLVM can't handle type=3 correctly.
   // Hopefully will be lowered properly in the future.
   // CHECK: store i32 0
   gi = __builtin_object_size((void*)0 + 0x1000, 3);
 }
+
+// CHECK: @test26
+void test26() {
+  struct { int v[10]; } t[10];
+
+  // CHECK: store i32 316
+  gi = __builtin_object_size(&t[1].v[11], 0);
+  // CHECK: store i32 312
+  gi = __builtin_object_size(&t[1].v[12], 1);
+  // CHECK: store i32 308
+  gi = __builtin_object_size(&t[1].v[13], 2);
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&t[1].v[14], 3);
+}
+
+struct Test27IncompleteTy;
+
+// CHECK: @test27
+void test27(struct Test27IncompleteTy *t) {
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
+  gi = __builtin_object_size(t, 0);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false)
+  gi = __builtin_object_size(t, 1);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true)
+  gi = __builtin_object_size(t, 2);
+  // Note: this is currently fixed at 0 because LLVM doesn't have sufficient
+  // data to correctly handle type=3
+  // CHECK: store i32 0
+  gi = __builtin_object_size(t, 3);
+
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false)
+  gi = __builtin_object_size(&test27, 0);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false)
+  gi = __builtin_object_size(&test27, 1);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true)
+  gi = __builtin_object_size(&test27, 2);
+  // Note: this is currently fixed at 0 because LLVM doesn't have sufficient
+  // data to correctly handle type=3
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&test27, 3);
+}
+
+// The intent of this test is to ensure that __builtin_object_size treats `&foo`
+// and `(T*)&foo` identically, when used as the pointer argument.
+// CHECK: @test28
+void test28() {
+  struct { int v[10]; } t[10];
+
+#define addCasts(s) ((char*)((short*)(s)))
+  // CHECK: store i32 360
+  gi = __builtin_object_size(addCasts(&t[1]), 0);
+  // CHECK: store i32 360
+  gi = __builtin_object_size(addCasts(&t[1]), 1);
+  // CHECK: store i32 360
+  gi = __builtin_object_size(addCasts(&t[1]), 2);
+  // CHECK: store i32 360
+  gi = __builtin_object_size(addCasts(&t[1]), 3);
+
+  // CHECK: store i32 356
+  gi = __builtin_object_size(addCasts(&t[1].v[1]), 0);
+  // CHECK: store i32 36
+  gi = __builtin_object_size(addCasts(&t[1].v[1]), 1);
+  // CHECK: store i32 356
+  gi = __builtin_object_size(addCasts(&t[1].v[1]), 2);
+  // CHECK: store i32 36
+  gi = __builtin_object_size(addCasts(&t[1].v[1]), 3);
+#undef addCasts
+}