[PATCH] Enhanced ignored-qualifiers checks for restrict

[Richard Smith]

On Tue, Oct 21, 2014 at 3:46 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> ----- Original Message -----
> > From: "Richard Smith" <richard at metafoo.co.uk>
> > To: "Hal Finkel" <hfinkel at anl.gov>
> > Cc: reviews+D5872+public+0417df57406a159b at reviews.llvm.org, "cfe
> commits" <cfe-commits at cs.uiuc.edu>, "Daniel Berlin"
> > <dberlin at dberlin.org>, "aaron ballman" <aaron.ballman at gmail.com>
> > Sent: Tuesday, October 21, 2014 3:47:03 PM
> > Subject: Re: [PATCH] Enhanced ignored-qualifiers checks for restrict
> >
> > On Tue, Oct 21, 2014 at 8:11 AM, Hal Finkel < hfinkel at anl.gov >
> > wrote:
> >
> >
> > ----- Original Message -----
> > > From: "Richard Smith" < richard at metafoo.co.uk >
> > > To: hfinkel at anl.gov , dberlin at dberlin.org , "aaron ballman" <
> > > aaron.ballman at gmail.com >, richard at metafoo.co.uk
> > > Cc: cfe-commits at cs.uiuc.edu
> > > Sent: Monday, October 20, 2014 4:17:00 PM
> > > Subject: Re: [PATCH] Enhanced ignored-qualifiers checks for
> > > restrict
> > >
> > > First off, more warnings for dubious uses of `restrict` seem
> > > valuable
> > > to me. The two warnings you suggest seem like good ideas.
> >
> > Thanks! There's more coming too...
> >
> > For the record, I'd like to work on enhancing our support for
> > restrict (as well as the hopefully-better C++ version being
> > developed, WG21 N4150 has the current wording in progress). I think
> > that enhancing our warnings on restrict is a good first step.
> >
> > > Some
> > > thoughts:
> > >
> > > I don't think cast expressions are the right thing to target here.
> > > The same issue applies to compound literals, new-expressions, and
> > > so
> > > on.
> >
> > Good point.
> >
> > > `restrict` only has meaning when used in "a declaration of an
> > > ordinary identifier that provides a means of designating an object
> > > P", so perhaps we should warn on it (at least at the top level) in
> > > all cases where it's not part of such a declaration (and can't be
> > > indirectly part of a declaration, such as if it's in a typedef or
> > > template argument).
> >
> > As a note, and personally I find the wording here a bit unclear, but
> > if you look at the rational document (WG14 N334), it is explicit
> > that the wording also is intended to allow restrict to be useful on
> > fields of structures. The identifier can be the thing that provides
> > access to the structure.
> >
> > But, generally speaking, I agree that the list of useful places is
> > much smaller than the list of useless places, and an exclusionary
> > design for the warning might make more sense. Do you have a
> > suggestion on how the warning should be implemented?
> >
> >
> > In SemaType's GetFullTypeForDeclarator, you have access to the
> > Context from the DeclSpec, which you can use to determine whether to
> > warn. (You'd probably want to add a check for whether the type from
> > the decl-specifiers is 'restrict', and also check each time you add
> > a pointer type whether that type introduces a 'restrict').
> >
> >
> >
> > > Taking the address of a `restrict`-qualified pointer has weird
> > > semantics, and perhaps deserves a warning. For instance:
> > >
> > > int *restrict a = &...;
> > > int **p = &a;
> > > *a = 1;
> > > int k = **p;
> > >
> > > ... is fine, even though `a` is accessed through a
> > > non-`restrict`-qualified pointer, because `*q` is based on `a`.
> >
> > This seems like a reasonable idea, I'll think about it. We probably
> > want a warning like:
> >
> > pointer access '**p' is based on restrict-qualified pointer 'a' in a
> > non-obvious way
> >
> > >
> > > Similarly, declaring an object of a type that has a non-top-level
> > > `restrict` qualifier also has weird semantics, and we should
> > > perhaps
> > > warn on that.
> >
> > Do you mean if someone tries to declare something like?
> > int * * restrict * restrict * x;
> >
> >
> > Sure. I was thinking of simpler things like
> >
> >
> > int *a = ...;
> > int **p = &a;
> > int *restrict *q = p;
> >
> >
> > // Now *p is based on 'a', which is 'restrict'ed because it can be
> > designated as the restrict-qualified pointer *p. But accessing 'a'
> > directly or through *p is fine, because those expressions are *also*
> > based on 'a'. So the 'restrict' here perhaps doesn't mean what its
> > author intended. Or maybe it does. *shrug*
> >
> >
> >
> > > Perhaps we should also warn on copying a `restrict`-qualified
> > > pointer
> > > to a non-`restrict`-qualified pointer, since that also seems like a
> > > very common error.
> >
> > I'm not sure about this one. It will be noisy, because as you noted
> > above, this is perfectly legal behavior (the non-restrict-qualified
> > variable's value simply becomes "based on" the restrict-qualified
> > pointer), and carries well-defined (if sometimes hard to deduce)
> > semantics.
> >
> >
> >
> > Consider this trivial example:
> >
> >
> > int *restrict a = malloc(sizeof(int) * 4);
> > for (int *p = a; p != a + 4; ++p)
> > *p = 0;
> > int result = a[n];
> >
> > This has undefined behavior because p is not based on a, and p is
> > used to modify an object accessed through a.
>
> Let me make sure we're on the same page...
>
> C99 6.7.3.1p3 In what follows, a pointer expression E is said to be based
> on object P if (at some
>
> sequence point in the execution of B prior to the evaluation of E)
> modifying P to point to
>
> a copy of the array object into which it formerly pointed would change the
> value of E.
>
> Note that ‘‘based’’ is defined only for expressions with pointer types.
>
> So here we're concerned with 'p' in '*p = 0' in the block that forms the
> body of the for loop. So p is not "based on" a in that loop because there
> is no sequence point in the loop where changing a to point to a copy would
> change the value of p.
>

Right.


> But, confusingly enough, maybe p is based on a inside the expressions of
> the for statement itself. So perhaps a generic way to say this is that a
> non-restrict-qualified pointer can become "based on" a restrict qualified
> pointer, but only within the block of the initial assignment (or containing
> blocks). Any use in a "child" (contained) block will cause undefined
> behavior.


Are you suggesting a change to the C standard here, or a way of reading it
which gives the above code defined behavior, or some extra
implementation-defined guarantee we might want to provide, or how we might
word a diagnostic to warn a user that they got this wrong?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141021/8af6c8af/attachment.html>