----- 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. 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. Sound right? Thanks again, Hal > > > Thanks again, > Hal > > > > > http://reviews.llvm.org/D5872 > > > > > > > > -- > Hal Finkel > Assistant Computational Scientist > Leadership Computing Facility > Argonne National Laboratory > > -- 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
