[PATCH] D12821: Allow for C's "writing off the end" idiom in __builtin_object_size

[George Burgess IV via cfe-commits]

george.burgess.iv added inline comments.

================
Comment at: lib/AST/ExprConstant.cpp:4457-4460
@@ -4434,1 +4456,6 @@
 
+    // Because we set the Base to be the MemberExpr instead of E->getBase(), the
+    // Offset should be from the MemberExpr instead of the MemberExpr's base.
+    if (Result.InvalidBase)
+      Result.Offset = CharUnits::Zero();
+
----------------
rsmith wrote:
> I think you should bail out above, with the base set to the `MemberExpr` and with an empty designator, rather than applying a member designator on top of a base that already refers to the member (and then needing to undo the effect on the offset here).
With the new approach, this change isn't even necessary anymore :)

================
Comment at: lib/AST/ExprConstant.cpp:6402
@@ +6401,3 @@
+  // that behaves this way.
+  if (End.InvalidBase && (Type & 1) != 0 &&
+      End.Designator.MostDerivedIsArrayElement &&
----------------
rsmith wrote:
> This only seems necessary when `Type` is 1; for type 3, returning 1 would be conservatively correct as a lower bound on the known-accessible storage, and is better than giving up here and evaluating the object size as 0.
Good catch

================
Comment at: lib/AST/ExprConstant.cpp:6403-6404
@@ +6402,4 @@
+  if (End.InvalidBase && (Type & 1) != 0 &&
+      End.Designator.MostDerivedIsArrayElement &&
+      End.Designator.MostDerivedArraySize < 2) {
+    // EM_FoldDesignator requires that all invalid bases be MemberExprs
----------------
rsmith wrote:
> Ideally, we should walk the designator and make sure that at each step we picked the last subobject (last array element, last field, ...) -- that is, we want to know that we're really looking at a trailing flexible array member, and not just a size-1 array in the middle of some object.
> 
> You should also check that the designator refers to the most-derived object (that is, that `Entries.size() == MostDerivedPathLength`), because given:
> 
>   struct A { char buffer[10]; };
>   struct B : A {};
>   struct C { B b[1]; } *c;
>   int m = __builtin_object_size(c->b[0], 1);
>   int n = __builtin_object_size((A*)c->b[0], 1);
> 
> ... we should presumably compute `m == -1` but `n == 10`, because for `n` the `A` subobject of the `B` object is known to have size 10, even though we're looking at a base subobject of a size-1 trailing array.
> You should also check that the designator refers to the most-derived object (that is, that Entries.size() == MostDerivedPathLength), because given

That's a fun case! Thanks for catching that.

> Ideally, we should walk the designator and make sure that at each step we picked the last subobject (last array element, last field, ...) -- that is, we want to know that we're really looking at a trailing flexible array member, and not just a size-1 array in the middle of some object

That's the goal of the AtEndOfObject check below. :) I guess it would fail with things like `union { char c[1]; int a; };` though. Added.


http://reviews.llvm.org/D12821