[PATCH] Enhanced ignored-qualifiers checks for restrict

[Richard Smith]

On Tue, Oct 21, 2014 at 4:33 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

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

Hmm, on re-reading 6.7.3.1, I've changed my mind on this. Consider the
sequence point after the initialization of 'a'. Changing 'a' to point to a
different object here would change the value of 'p' within the loop, so all
is fine here.

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?
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141021/5b147009/attachment.html>