[PATCH] D133108: [clang] Rework IsTailPaddedMemberArray into isFlexibleArrayMemberExpr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, though this should probably have a release note for it (we can augment the release note when we make further changes in this area).



================
Comment at: clang/test/Sema/unbounded-array-bounds.c:101
+  char tail[1];  // addr16-note {{declared here}} addr32-note {{declared here}}
+} fam1;
+
----------------
serge-sans-paille wrote:
> msebor wrote:
> > msebor wrote:
> > > There's a difference between the sizes of `fam1` and `fam` that makes accesses to the four leading elements of `fam1.tail` strictly in bounds, while no access to either `fam.tail` or `fam0.tail` is (`sizeof fam` is the same as `sizeof int` while `sizeof fam1` is equal to `sizeof (int[2])` on common targets).  It would be helpful to capture that difference in the tests, both for the warning and for `__builtin_object_size`.
> > > 
> > > There should also be a difference between accessing elements of an object of an initialized struct with a flexible array member (i.e., one whose size is known) and those of an object that's only declared but that's defined in some other translation unit.  Since the size of the object is determined by its initializer, it should be reflected in `__builtin_object_size` and accesses to it checked by `-Warray-bounds`.  The size of the latter object is unknown it must be assumed to be `PTRDIFF_MAX - sizeof (int) - 1`.  It would also be helpful to add tests for these cases.
> > > 
> > > As far as I can see, none of these cases seems to be handled quite right on trunk.  For example, the size of `s` below should be 8 but Clang evaluates `__builtin_object_size(&s, N)` to 4, without diagnosing any past-the-end accesses to `s.a`:
> > > ```
> > > struct S {
> > >   int n;
> > >   char a[];
> > > } s = { 1, { 2, 3, 4, 5 } };
> > > ```
> > I opened [[ https://github.com/llvm/llvm-project/issues/57860 | PR #57860 ]] to better show what I mean.
> Agreed. I suggest we discuss that in this PR, while me merge this "code cleanup with enhancement", if @aaron.ballman agrees :-)
+1 -- there are bugs there to be fixed, but handling that separately from this cleanup seems like it will be easier to review and discuss.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133108/new/

https://reviews.llvm.org/D133108