----- 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 6:33:03 PM > Subject: Re: [PATCH] Enhanced ignored-qualifiers checks for restrict > > 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. > > > 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
