<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Oct 21, 2014 at 4:33 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Tue, Oct 21, 2014 at 3:46 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>----- Original Message -----<br>
> From: "Richard Smith" <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><br>
</span><span>> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>><br>
> Cc: <a href="mailto:reviews%2BD5872%2Bpublic%2B0417df57406a159b@reviews.llvm.org" target="_blank">reviews+D5872+public+0417df57406a159b@reviews.llvm.org</a>, "cfe commits" <<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>>, "Daniel Berlin"<br>
> <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>>, "aaron ballman" <<a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@gmail.com</a>><br>
> Sent: Tuesday, October 21, 2014 3:47:03 PM<br>
> Subject: Re: [PATCH] Enhanced ignored-qualifiers checks for restrict<br>
><br>
</span><span>> On Tue, Oct 21, 2014 at 8:11 AM, Hal Finkel < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> ><br>
> wrote:<br>
><br>
><br>
> ----- Original Message -----<br>
> > From: "Richard Smith" < <a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a> ><br>
</span><div><div>> > To: <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> , <a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a> , "aaron ballman" <<br>
> > <a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@gmail.com</a> >, <a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a><br>
> > Cc: <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> > Sent: Monday, October 20, 2014 4:17:00 PM<br>
> > Subject: Re: [PATCH] Enhanced ignored-qualifiers checks for<br>
> > restrict<br>
> ><br>
> > First off, more warnings for dubious uses of `restrict` seem<br>
> > valuable<br>
> > to me. The two warnings you suggest seem like good ideas.<br>
><br>
> Thanks! There's more coming too...<br>
><br>
> For the record, I'd like to work on enhancing our support for<br>
> restrict (as well as the hopefully-better C++ version being<br>
> developed, WG21 N4150 has the current wording in progress). I think<br>
> that enhancing our warnings on restrict is a good first step.<br>
><br>
> > Some<br>
> > thoughts:<br>
> ><br>
> > I don't think cast expressions are the right thing to target here.<br>
> > The same issue applies to compound literals, new-expressions, and<br>
> > so<br>
> > on.<br>
><br>
> Good point.<br>
><br>
> > `restrict` only has meaning when used in "a declaration of an<br>
> > ordinary identifier that provides a means of designating an object<br>
> > P", so perhaps we should warn on it (at least at the top level) in<br>
> > all cases where it's not part of such a declaration (and can't be<br>
> > indirectly part of a declaration, such as if it's in a typedef or<br>
> > template argument).<br>
><br>
> As a note, and personally I find the wording here a bit unclear, but<br>
> if you look at the rational document (WG14 N334), it is explicit<br>
> that the wording also is intended to allow restrict to be useful on<br>
> fields of structures. The identifier can be the thing that provides<br>
> access to the structure.<br>
><br>
> But, generally speaking, I agree that the list of useful places is<br>
> much smaller than the list of useless places, and an exclusionary<br>
> design for the warning might make more sense. Do you have a<br>
> suggestion on how the warning should be implemented?<br>
><br>
><br>
> In SemaType's GetFullTypeForDeclarator, you have access to the<br>
> Context from the DeclSpec, which you can use to determine whether to<br>
> warn. (You'd probably want to add a check for whether the type from<br>
> the decl-specifiers is 'restrict', and also check each time you add<br>
> a pointer type whether that type introduces a 'restrict').<br>
><br>
><br>
><br>
> > Taking the address of a `restrict`-qualified pointer has weird<br>
> > semantics, and perhaps deserves a warning. For instance:<br>
> ><br>
> > int *restrict a = &...;<br>
> > int **p = &a;<br>
> > *a = 1;<br>
> > int k = **p;<br>
> ><br>
> > ... is fine, even though `a` is accessed through a<br>
> > non-`restrict`-qualified pointer, because `*q` is based on `a`.<br>
><br>
> This seems like a reasonable idea, I'll think about it. We probably<br>
> want a warning like:<br>
><br>
> pointer access '**p' is based on restrict-qualified pointer 'a' in a<br>
> non-obvious way<br>
><br>
> ><br>
> > Similarly, declaring an object of a type that has a non-top-level<br>
> > `restrict` qualifier also has weird semantics, and we should<br>
> > perhaps<br>
> > warn on that.<br>
><br>
> Do you mean if someone tries to declare something like?<br>
> int * * restrict * restrict * x;<br>
><br>
><br>
> Sure. I was thinking of simpler things like<br>
><br>
><br>
> int *a = ...;<br>
> int **p = &a;<br>
> int *restrict *q = p;<br>
><br>
><br>
> // Now *p is based on 'a', which is 'restrict'ed because it can be<br>
> designated as the restrict-qualified pointer *p. But accessing 'a'<br>
> directly or through *p is fine, because those expressions are *also*<br>
> based on 'a'. So the 'restrict' here perhaps doesn't mean what its<br>
> author intended. Or maybe it does. *shrug*<br>
><br>
><br>
><br>
> > Perhaps we should also warn on copying a `restrict`-qualified<br>
> > pointer<br>
> > to a non-`restrict`-qualified pointer, since that also seems like a<br>
> > very common error.<br>
><br>
> I'm not sure about this one. It will be noisy, because as you noted<br>
> above, this is perfectly legal behavior (the non-restrict-qualified<br>
> variable's value simply becomes "based on" the restrict-qualified<br>
> pointer), and carries well-defined (if sometimes hard to deduce)<br>
> semantics.<br>
><br>
><br>
><br>
> Consider this trivial example:<br>
><br>
><br>
> int *restrict a = malloc(sizeof(int) * 4);<br>
> for (int *p = a; p != a + 4; ++p)<br>
> *p = 0;<br>
> int result = a[n];<br>
><br>
> This has undefined behavior because p is not based on a, and p is<br>
> used to modify an object accessed through a.<br>
<br>
</div></div>Let me make sure we're on the same page...<br>
<br>
C99 6.7.3.1p3 In what follows, a pointer expression E is said to be based on object P if (at some<br>
<br>
sequence point in the execution of B prior to the evaluation of E) modifying P to point to<br>
<br>
a copy of the array object into which it formerly pointed would change the value of E.<br>
<br>
Note that ‘‘based’’ is defined only for expressions with pointer types.<br>
<br>
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.<br></blockquote><div><br></div></div></div><div>Right.</div></div></div></div></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.</blockquote><div><br></div></span><div>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? </div></div></div></div>
</blockquote></div><br></div></div>