[PATCH] Enhanced ignored-qualifiers checks for restrict

[Richard Smith]

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.


> Thanks again,
> Hal
>
> >
> > http://reviews.llvm.org/D5872
> >
> >
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141021/5cf75fad/attachment.html>