[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 6:33:03 PM
> Subject: Re: [PATCH] Enhanced ignored-qualifiers checks for restrict
> 
> 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?

The latter... I'm trying to figure out how we might word a diagnostic about this, and explain it to users -- and anyone else on this list who's reading along ;)

Thanks again,
Hal

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory