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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
