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