[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').
> 
> 
> 
> > 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.

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.

Sound right?

Thanks again,
Hal

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