On Tue, Oct 21, 2014 at 4:33 PM, Richard Smith <[email protected]> wrote:
> On Tue, Oct 21, 2014 at 3:46 PM, Hal Finkel <[email protected]> wrote: > >> ----- Original Message ----- >> > From: "Richard Smith" <[email protected]> >> > To: "Hal Finkel" <[email protected]> >> > Cc: [email protected], "cfe >> commits" <[email protected]>, "Daniel Berlin" >> > <[email protected]>, "aaron ballman" <[email protected]> >> > 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 < [email protected] > >> > wrote: >> > >> > >> > ----- Original Message ----- >> > > From: "Richard Smith" < [email protected] > >> > > To: [email protected] , [email protected] , "aaron ballman" < >> > > [email protected] >, [email protected] >> > > Cc: [email protected] >> > > 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? >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
