[PATCH] Enhanced ignored-qualifiers checks for restrict

Wed Oct 22 11:19:20 PDT 2014

On Wed, Oct 22, 2014 at 1:27 AM, 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').
>
> This is useful, thanks! However, for casts, I'd likely still want access
> to the FromTy, but more than that:
>
> char foo(float * __restrict__ x) {
>   char * __restrict__ y = (char * __restrict__) x;
>   return *y;
> }
>
> Should we warn here about the __restrict__ in the cast? I think this is
> questionable (because it is technically unnecessary), but:
>
> typedef float * __restrict__ floatp;
> typedef char * __restrict__ charp;
>
> char goo(floatp x) {
>   charp y = (charp) x;
>   return *y;
> }
>
> I definitely think that we should not warn here about the fact that charp
> is restrict-qualified in the cast (because I don't want to force anyone to
> manually expand a typedef to get around a warning). However, I certainly do
> want to warn on restrict-qualified typedefs generally: *((charp) p) += 1
> should get a warning.
>
> so I may need even more context than the TheContext in DeclSpec currently
> provides?
>

This is possible, and may not be too hard: you could add a list of the
CastExprs you're considering warning about to the ExprEvaluationContext,
remove them from the list if you see they're used as the initializer for a
restrict-qualified pointer, and warn on them when you pop the context.

I suppose you should only do this for some flavors of initialization,
though, and in particular not for function parameter initialization.
Consider:

char *blah(charp x); // maybe warn here: restrict has no effect on a
parameter in a function declaration?
void boo(char *p) {
  { // 1:
    char *q = blah(p); // ok, restrict on parameter only affects body of
blah
  }
  { // 2:
    char *q = blah((charp)p); // warn, or not?
  }
  { // 3:
    charp r = (charp)p;
    char *q = blah(r);
  }
}

Now, suppose blah returns its argument. In blocks 1 and 2, it's permissible
to use both p and q after the call. In block 3, it would be UB.

So I think we should warn on the non-defining declaration of 'blah'. And we
should warn on block 2 because we used a restrict-qualified cast but the
restrict was meaningless. But we should not warn about the cast in 3. Make
sense?

Having said that, I wonder if we should warn about 3 regardless (probably
under a different warning flag) for having an indirect use of 'restrict'
through a typedef. It's not obvious to a reader of the code in 3 that the
variable is in fact declared 'restrict', and it has a
potentially-surprising effect on the semantics of the block. There's a
simple syntactic transformation we could suggest to make this obvious to
readers of the code (and to suppress such a warning):

  charp restrict r = (charp)p;

Perhaps people would always put the word 'restrict' in the name of the
typedef, making this unnecessary. Do you have much experience with how
these kinds of typedefs might be used?

Thanks again,
> Hal
>
> >
> >
> >
> > > 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
> >
> >
>
> --
> 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/20141022/d4da95cf/attachment.html>