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

479

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

498–502

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.

866

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.

1983–1986

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.

4003–4008

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.

4626–4632

Why do you hate comments? :)

4879–4907

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.

4879–4907

[...] 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?)

4980–4987

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.

6299

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

6331–6336

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).

6403

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

498–502

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

499

DesignatorFold it is -- thanks for the suggestions.

866

SGTM. Backed it out.

4626–4632

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

4879–4913

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.

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).

6299

Thanks for the heads-up :)

6331–6336

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

6403

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

       MostDerivedPathLength = Entries.size();
     }
     void diagnosePointerArithmetic(EvalInfo &Info, const Expr *E, uint64_t N);
-    /// Add N to the address of this subobject.
+
+    /// Add N to the index of this subobject.
     void adjustIndex(EvalInfo &Info, const Expr *E, uint64_t N) {
       if (Invalid) return;
       if (MostDerivedPathLength == Entries.size() && MostDerivedArraySize) {

                             const LValue &This, const Expr *E,
                             bool AllowNonLiteralTypes = false);
 static bool EvaluateLValue(const Expr *E, LValue &Result, EvalInfo &Info);
-static bool EvaluatePointer(const Expr *E, LValue &Result, EvalInfo &Info);
+static bool EvaluateOutermostMember(const Expr *E, EvalInfo &Info,
+                                    QualType &Result, CharUnits &Offset,
+                                    bool &IsInnermost);
+static bool EvaluatePointerWithOffset(const Expr *E, LValue &Result,
+                                      CharUnits &Offset, EvalInfo &Info,
+                                      bool UseStrictCastingRules = true);
+static bool EvaluatePointer(const Expr *E, LValue &Result, EvalInfo &Info,
+                            bool UseStrictCastingRules = true);
 static bool EvaluateMemberPointer(const Expr *E, MemberPtr &Result,
                                   EvalInfo &Info);
 static bool EvaluateTemporary(const Expr *E, LValue &Result, EvalInfo &Info);

   : public ExprEvaluatorBase<PointerExprEvaluator> {
   LValue &Result;
 
+  /// Any additional offset from Result that can't be accurately recorded as an
+  /// index in Result.Entries.back()
+  CharUnits &AddedOffset;
+
+  /// Though reinterpret_cast is not allowed in constexpr evaluation, we
+  /// sometimes use PointerExprEvaluator for non-constexpr things (intrinsics),
+  /// in which we can allow BitCasts to char*/etc. purely for pointer
+  /// arithmetic.
+  bool UseStrictCastingRules;
+
   bool Success(const Expr *E) {
     Result.set(E);
     return true;
   }
-public:
 
-  PointerExprEvaluator(EvalInfo &info, LValue &Result)
-    : ExprEvaluatorBaseTy(info), Result(Result) {}
+public:
+  PointerExprEvaluator(EvalInfo &info, LValue &Result, CharUnits &AddedOffset,
+                       bool UseStrictCastingRules)
+      : ExprEvaluatorBaseTy(info), Result(Result), AddedOffset(AddedOffset),
+        UseStrictCastingRules(UseStrictCastingRules) {}
 
   bool Success(const APValue &V, const Expr *E) {
     Result.setFrom(Info.Ctx, V);

   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);

 };
 } // end anonymous namespace
 
-static bool EvaluatePointer(const Expr* E, LValue& Result, EvalInfo &Info) {
+/// Evaluates a pointer expression that may have offsets that are not a
+/// multiple of sizeof(*ptr), while trying to fold these offsets into an index.
+/// e.g.
+/// short s[2];
+/// // AddedOffset == 1, Result.Designator.Entries.back().Index == 0
+/// constexpr short *sp1 = (char*)&s[0] + 1;
+/// // AddedOffset == 0, Result.Designator.Entries.back().Index == 1
+/// constexpr short *sp2 = (char*)&s[0] + sizeof(short);
+///
+/// The additional offset **is included** in Result.Offset. AddedOffset should
+/// only be used if knowledge of just this added offset is necessary.
+static bool EvaluatePointerWithOffset(const Expr *E, LValue &Result,
+                                      CharUnits &AddedOffset, EvalInfo &Info,
+                                      bool StrictCasts) {
   assert(E->isRValue() && E->getType()->hasPointerRepresentation());
-  return PointerExprEvaluator(Info, Result).Visit(E);
+  AddedOffset = CharUnits::Zero();
+  return PointerExprEvaluator(Info, Result, AddedOffset, StrictCasts).Visit(E);
+}
+
+/// Attempts to evaluate a pointer expression. Fails and invalidates Result's
+/// Designator if there's any offset that can't be represented as an array
+/// index. (e.g. Evaluate(&I32Array+1) is okay, Evaluate((char*)&I32Array+1)
+/// is not.)
+static bool EvaluatePointer(const Expr *E, LValue &Result, EvalInfo &Info,
+                            bool StrictCasts) {
+  CharUnits Offset;
+  if (!EvaluatePointerWithOffset(E, Result, Offset, Info, StrictCasts))
+    return false;
+  if (!Offset.isZero()) {
+    Result.Designator.setInvalid();
+    return false;
+  }
+  return true;
 }
 
 bool PointerExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {

   if (IExp->getType()->isPointerType())
     std::swap(PExp, IExp);
 
-  bool EvalPtrOK = EvaluatePointer(PExp, Result, Info);
+  bool EvalPtrOK = EvaluatePointer(PExp, Result, Info, UseStrictCastingRules);
   if (!EvalPtrOK && !Info.keepEvaluatingAfterFailure())
     return false;
 
   llvm::APSInt Offset;
   if (!EvaluateInteger(IExp, Offset, Info) || !EvalPtrOK)
     return false;
 
-  int64_t AdditionalOffset = getExtValue(Offset);
+  int64_t IndexOffset = getExtValue(Offset);
   if (E->getOpcode() == BO_Sub)
-    AdditionalOffset = -AdditionalOffset;
+    IndexOffset = -IndexOffset;
 
   QualType Pointee = PExp->getType()->castAs<PointerType>()->getPointeeType();
-  return HandleLValueArrayAdjustment(Info, E, Result, Pointee,
-                                     AdditionalOffset);
+
+  // Special case: (char*)Foo.Bar[2] + sizeof(Foo.Bar[2]).
+  // We need to adjust the array index we're handing off in that case.
+  QualType ArrayTy = Result.Designator.MostDerivedType;
+  if (!ArrayTy.isNull() && !ArrayTy->isIncompleteType() && ArrayTy != Pointee) {
+    CharUnits PointeeEltSize;
+    if (!HandleSizeof(Info, PExp->getExprLoc(), Pointee, PointeeEltSize))
+      return false;
+
+    CharUnits ArrayEltSize;
+    if (!HandleSizeof(Info, PExp->getExprLoc(), ArrayTy, ArrayEltSize))
+      return false;
+
+    // Additionally, because LValuePathEntry wasn't made to really handle byte
+    // offsets, we need to keep the LValue Offset up-to-date (so casting e.g.
+    // BaseToDerived works as intended), but we also need to lie a bit about the
+    // array index.
+    //
+    // Example:
+    // (char*)Foo.Bar[1] + 2 // (assuming sizeof(Foo.Bar[1]) > 2)
+    // Will have an index of 1 and an offset of offsetof(Foo.Bar[1]) + 2.
+    if (PointeeEltSize != ArrayEltSize) {
+      CharUnits ByteOffset = IndexOffset * PointeeEltSize + AddedOffset;
+      CharUnits Rem = CharUnits::fromQuantity(ByteOffset % ArrayEltSize);
+      Result.Offset += Rem - AddedOffset;
+      AddedOffset = Rem;
+      IndexOffset = ByteOffset / ArrayEltSize;
+    }
+
+    // HandleLValueArrayAdjustment takes the type of the array
+    Pointee = ArrayTy;
+  }
+
+  return HandleLValueArrayAdjustment(Info, E, Result, Pointee, IndexOffset);
 }
 
 bool PointerExprEvaluator::VisitUnaryAddrOf(const UnaryOperator *E) {

     // 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()) {
+    if (UseStrictCastingRules && !E->getType()->isVoidPointerType()) {
       Result.Designator.setInvalid();
       if (SubExpr->getType()->isVoidPointerType())
         CCEDiag(E, diag::note_constexpr_invalid_cast)

 
 /// Retrieves the "underlying object type" of the given expression,
 /// as used by __builtin_object_size.
-static QualType getObjectType(APValue::LValueBase B) {
+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();

   return QualType();
 }
 
+namespace {
+/// We need an overall substantially more relaxed evaluator for
+/// __builtin_object_size than we need for constexprs. Some examples of GCC's
+/// behavior...
+/// struct A { struct { int is[10]; } b[10]; } a[10];
+/// __builtin_object_size(&a[-1].b[-1].is[0], 1) == 40
+/// __builtin_object_size(&a[-1].b[-1].is[-1], 1) == 0
+/// __builtin_object_size(&a[10].b[10].is[0], 1) == 40
+/// __builtin_object_size((char*)&a[0].b[0] + sizeof(a[0].b[0]), 1) == 40
+/// __builtin_object_size(&((struct A *)nullptr)->b[0], 1) == 40
+class OutermostMemberEvaluator
+    : public ConstStmtVisitor<OutermostMemberEvaluator, bool> {
+  EvalInfo &Info;
+  QualType &Result;
+
+  // Identical to Offset in PointerExprEvaluator
+  CharUnits &Offset;
+
+  // Is the outermost field also the innermost field? (e.g. in &foo)
+  bool &IsInnermost;
+
+  bool handleOffsetValue(const Expr *PExp, int64_t Offset);
+  bool handleOffsetExpr(const Expr *PExp, const Expr *IExp, bool Negative);
+
+public:
+  OutermostMemberEvaluator(EvalInfo &Info, QualType &Result, CharUnits &Offset,
+                           bool &IsInnermost)
+      : Info(Info), Result(Result), Offset(Offset), IsInnermost(IsInnermost) {}
+
+  bool VisitBinaryOperator(const BinaryOperator *E);
+  bool VisitDeclRefExpr(const DeclRefExpr *E);
+  bool VisitMemberExpr(const MemberExpr *E);
+
+  bool VisitArraySubscriptExpr(const ArraySubscriptExpr *E) {
+    return handleOffsetExpr(E->getBase(), E->getIdx(), false);
+  }
+  bool VisitCastExpr(const CastExpr *E) { return Visit(E->getSubExpr()); }
+  bool VisitParenExpr(const ParenExpr *E) { return Visit(E->getSubExpr()); }
+  bool VisitParenListExpr(const ParenListExpr *E) {
+    return Visit(E->getExpr(E->getNumExprs() - 1));
+  }
+  bool VisitUnaryAddrOf(const UnaryOperator *E) {
+    return Visit(E->getSubExpr());
+  }
+};
+} // anonymous namespace
+
+bool OutermostMemberEvaluator::handleOffsetExpr(const Expr *PExp,
+                                                const Expr *IExp,
+                                                bool Negative) {
+  APSInt Additional;
+  if (!EvaluateInteger(IExp, Additional, Info))
+    return false;
+
+  int64_t AdditionalUnits = getExtValue(Additional);
+  if (Negative)
+    AdditionalUnits = -AdditionalUnits;
+
+  return handleOffsetValue(PExp, AdditionalUnits);
+}
+
+bool OutermostMemberEvaluator::handleOffsetValue(const Expr *PExp,
+                                                 int64_t Amount) {
+  QualType PtrType = PExp->getType();
+  assert(PtrType->isPointerType());
+
+  // We need to be able to handle cases where we're not given selectors. The
+  // most natural type to take from those is the type most local to the
+  // pointer.
+  Result = PtrType;
+
+  if (!EvaluateOutermostMember(PExp, Info, Result, Offset, IsInnermost))
+    return false;
+
+  QualType PointeeType = PtrType->castAs<PointerType>()->getPointeeType();
+  CharUnits PointeeSize;
+  if (!HandleSizeof(Info, PExp->getExprLoc(), PointeeType, PointeeSize))
+    return false;
+
+  Offset += PointeeSize * Amount;
+  return true;
+}
+
+static bool EvaluateOutermostMember(const Expr *E, EvalInfo &Info,
+                                    QualType &Result, CharUnits &Offset,
+                                    bool &IsInnermost) {
+  IsInnermost = true;
+  return OutermostMemberEvaluator(Info, Result, Offset, IsInnermost).Visit(E);
+}
+
+bool OutermostMemberEvaluator::VisitDeclRefExpr(const DeclRefExpr *E) {
+  Result = E->getType();
+  return true;
+}
+
+bool OutermostMemberEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
+  auto Opcode = E->getOpcode();
+  if (Opcode == BO_Comma)
+    return Visit(E->getRHS());
+  if (Opcode != BO_Add && Opcode != BO_Sub)
+    return ConstStmtVisitor::VisitBinaryOperator(E);
+
+  bool Negative = Opcode == BO_Sub;
+  const Expr *PExp = E->getLHS();
+  const Expr *IExp = E->getRHS();
+  if (IExp->getType()->isPointerType())
+    std::swap(PExp, IExp);
+
+  return handleOffsetExpr(PExp, IExp, Negative);
+}
+
+bool OutermostMemberEvaluator::VisitMemberExpr(const MemberExpr *E) {
+  IsInnermost = false;
+  Result = E->getType();
+  return true;
+}
+
+/// Evaluates __builtin_object_size
 bool IntExprEvaluator::TryEvaluateBuiltinObjectSize(const CallExpr *E,
                                                     unsigned Type) {
-  // Determine the denoted object.
-  LValue Base;
+  QualType FieldType;
+  CharUnits BaseOffset;
   {
     // 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))
-      return false;
-  }
 
-  CharUnits BaseOffset = Base.getLValueOffset();
+    bool RequireFullEvaluation = true;
+    if (Type & 1) {
+      bool IsInnermost;
+      if (!EvaluateOutermostMember(E->getArg(0), Info, FieldType, BaseOffset,
+                                   IsInnermost))
+        return Error(E);
 
-  // If we point to before the start of the object, there are no
-  // accessible bytes.
-  if (BaseOffset < CharUnits::Zero())
-    return Success(0, E);
+      // If (Type & 1), we assume that any container(s) of the subobject are
+      // valid. However, if we're not dealing with a subobject, we need to
+      // act more or less as though the LSB of Type was unset to begin with.
+      RequireFullEvaluation = IsInnermost;
+    }
 
-  // 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())
-    return Error(E);
+    if (RequireFullEvaluation) {
+      LValue Base;
+      CharUnits Discard;
+      if (!EvaluatePointerWithOffset(E->getArg(0), Base, Discard, Info, false))
+        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();
+      // 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())
+        return Error(E);
+
+      FieldType = GetObjectType(Base.getLValueBase());
+      BaseOffset = Base.getLValueOffset();
     }
   }
 
-  // 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;
+  if (BaseOffset.isNegative())
+    return Success(0, E);
 
-  // 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) {
-    // We're already pointing at the end of the object.
-    AmountToAdd = 0;
-  }
-
-  if (End.Designator.MostDerivedType->isIncompleteType() ||
-      End.Designator.MostDerivedType->isFunctionType())
+  if (FieldType.isNull() || FieldType->isIncompleteType() ||
+      FieldType->isFunctionType())
     return Error(E);
 
-  if (!HandleLValueArrayAdjustment(Info, E, End, End.Designator.MostDerivedType,
-                                   AmountToAdd))
-    return false;
+  CharUnits EndOffset;
+  if (!HandleSizeof(Info, E->getExprLoc(), FieldType, EndOffset))
+    return Error(E);
 
-  auto EndOffset = End.getLValueOffset();
   if (BaseOffset > EndOffset)
     return Success(0, E);
 
+  // Note: When this unifies N types at some point in the future, Type=2|3 needs
+  // to be min_element(sizes), Type=0|1 needs to be max_element(sizes)
   return Success(EndOffset - BaseOffset, E);
 }
 

test/CXX/expr/expr.const/p2-0x.cpp

     constexpr B *p = a[0] + 4; // expected-error {{constant expression}} expected-note {{element 4 of array of 3 elements}}
     B b = {};
     constexpr A *pa = &b + 1; // expected-error {{constant expression}} expected-note {{base class of pointer past the end}}
-    constexpr B *pb = (B*)((A*)&b + 1); // expected-error {{constant expression}} expected-note {{derived class of pointer past the end}}
+    constexpr B *pb = (B*)((A*)&b + 4); // expected-error {{constant expression}} expected-note {{derived class of pointer past the end}}
     constexpr const int *pn = &(&b + 1)->n; // expected-error {{constant expression}} expected-note {{field of pointer past the end}}
     constexpr B *parr = &a[3][0]; // expected-error {{constant expression}} expected-note {{array element of pointer past the end}}
 

test/CodeGen/object-size.c

   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]; };
 
 // 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, 3);
 }
 
-
 // PR24493 -- ICE if __builtin_object_size called with NULL and (Type & 1) != 0
 // CHECK @test24
 void test24() {

   // CHECK: store i32 0
   gi = __builtin_object_size((void*)0 + 0x1000, 3);
 }
+
+// CHECK: @test26
+void test26(struct Test23Ty *p) {
+  struct { int t[10]; } t[10];
+
+  // CHECK: store i32 356
+  gi = __builtin_object_size((char*)&t[1].t[1], 0);
+  // CHECK: store i32 36
+  gi = __builtin_object_size((char*)&t[1].t[1], 1);
+  // CHECK: store i32 356
+  gi = __builtin_object_size((char*)&t[1].t[1], 2);
+  // CHECK: store i32 36
+  gi = __builtin_object_size((char*)&t[1].t[1], 3);
+}
+
+// CHECK: @test27
+void test27() {
+  struct { int t[10]; } t[10];
+
+  // CHECK: store i32 359
+  gi = __builtin_object_size((char*)&t[1].t[0]+1, 0);
+  // CHECK: store i32 39
+  gi = __builtin_object_size((char*)&t[1].t[0]+1, 1);
+  // CHECK: store i32 359
+  gi = __builtin_object_size((char*)&t[1].t[0]+1, 2);
+  // CHECK: store i32 39
+  gi = __builtin_object_size((char*)&t[1].t[0]+1, 3);
+}
+
+// CHECK: @test28
+void test28() {
+  struct { int t[10]; } t[10];
+
+  // CHECK: store i32 356
+  gi = __builtin_object_size(&t[0].t[11], 0);
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&t[0].t[12], 1);
+  // CHECK: store i32 348
+  gi = __builtin_object_size(&t[0].t[13], 2);
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&t[0].t[14], 3);
+
+  // CHECK: store i32 364
+  gi = __builtin_object_size(&t[1].t[-1], 0);
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&t[1].t[-2], 1);
+  // CHECK: store i32 372
+  gi = __builtin_object_size(&t[1].t[-3], 2);
+  // CHECK: store i32 0
+  gi = __builtin_object_size(&t[1].t[-4], 3);
+}
+
+struct Test30IncompleteTy;
+
+// CHECK: @test29
+void test29(struct Test30IncompleteTy *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(&test29, 0);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false)
+  gi = __builtin_object_size(&test29, 1);
+  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true)
+  gi = __builtin_object_size(&test29, 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(&test29, 3);
+}