[PATCH] Enhanced ignored-qualifiers checks for restrict

[Hal Finkel]

----- 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?

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