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