[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:13384
       case Stmt::MemberExprClass: {
         expr = cast<MemberExpr>(expr)->getBase();
         break;
----------------
ilya wrote:
> ebevhan wrote:
> > ilya wrote:
> > > rsmith wrote:
> > > > ilya wrote:
> > > > > rsmith wrote:
> > > > > > Hmm, don't we need to do different things for dot and arrow in this case?
> > > > > There are several test cases for an out of bounds access on an array member using dot and arrow operators in array-bounds.cpp. Do you have a specific test case for which you think the code is broken?
> > > > > There are several test cases for an out of bounds access on an array member using dot and arrow operators in array-bounds.cpp. Do you have a specific test case for which you think the code is broken?
> > > > 
> > > > Sure. There's a false negative for this:
> > > > 
> > > > ```
> > > > struct A { int n; };
> > > > A *a[4];
> > > > int *n = &a[4]->n;
> > > > ```
> > > > 
> > > > ... because we incorrectly visit the left-hand side of the `->` with `AllowOnePastEnd == 1`. The left-hand side of `->` is subject to lvalue-to-rvalue conversion, so can't be one-past-the-end regardless of the context in which the `->` appears.
> > > > ... because we incorrectly visit the left-hand side of the -> with AllowOnePastEnd == 1. The left-hand side of -> is subject to lvalue-to-rvalue conversion, so can't be one-past-the-end regardless of the context in which the -> appears.
> > > 
> > > Thanks for clarifying. So basically we don't want to allow one-past-end for MemberExpr?
> > > Thanks for clarifying. So basically we don't want to allow one-past-end for MemberExpr?
> > 
> > I think the point is rather that when the MemberExpr isArrow, we want to think of it as performing an implicit dereference first, and thus we should do `AllowOnePastEnd--` before recursing if that is the case.
> > I think the point is rather that when the MemberExpr isArrow, we want to think of it as performing an implicit dereference first, and thus we should do AllowOnePastEnd-- before recursing if that is the case.
> 
> @ebevhan and I have discussed this offline, and we believe that there's no reason to allow an //address-of// of a member of a one past-the-end value, using either '.' or '->' operators, regardless of the fact that referencing a member of a one past-the-end value using '.' is merely a pointer arithmetic and doesn't imply dereferencing, as with operator '->'.
> 
> We couldn't find any reference in the [[ http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4659.pdf| C++ 2017 N4659 ]] regarding member access of a one past-the-end value. Only mentions of one past-the-end in the context of iterators (27.2.1 p. 7) and pointer arithmetic (8.3.1 p. 3).
> 
> [[ http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf | ISO C99 ]] describes in 6.5.3.2 p. 3 that if the operand to the '&' operator is the result of an unary '*' operator (or array subscript, implictly), the two operators "cancel" each other, but it says nothing about the case when there's a member expression "in-between", (the member expression section, 6.5.2.3, says nothing about that either).
> 
> @rsmith, what do you think?
Let's give this a go. I'm a little concerned that people might write things like:

```
struct X { int n; };
X x[32];
int *end = &x[32].n;
```

I don't think that has defined behavior in C++, because there isn't an `X` object so there isn't an `n` member to refer to, but I think it's probably OK in C (hard to say in both cases, though, due to underspecification in both standards). Nonetheless, such code is at least questionable. I don't think we have a good way to determine how well this warning will work other than trying it, so let's do that.

Please be prepared to revert and do something more conservative if we find this fires a lot on somewhat-reasonable code :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71714