Page MenuHomePhabricator

Enhanced ignored-qualifiers checks for restrict
Needs ReviewPublic

Authored by hfinkel on Oct 20 2014, 1:05 PM.

Details

Summary

We already include the restrict qualifier in the -Wignored-qualifiers checks, and so we'll produce a warning when -Wignored-qualifiers is in effect for a function declared with a restrict-qualified return type:

warning: 'restrict' type qualifier on return type has no effect [-Wignored-qualifiers]
   int * restrict f8a(void);
         ^~~~~~~~~

but we don't warn about restrict-qualified types being used a return types generally. We also don't warn about restrict-qualified types in case expressions (which also don't convey any pointer-aliasing information because of how restrict's semantics are defined).

restrict's semantics are subtle, and there is a lot of misunderstanding about exactly how restrict can be used. Users often believe they can put restrict on function return types and in cast expressions to express pointer-aliasing information, and they can't (although the resulting code is legal).

This patch does two things. First, it adds a warning about indirectly-restrict-qualified function return types. For example, the code:

typedef float * restrict floatp;
typedef floatp realp;
realp f8c(void);

will yield (when compiling with -Wignored-qualifiers):

warning: restrict-qualified function return type has no effect [-Wignored-qualifiers]
realp f8c(void);
^~~~~
note: 'realp' declared here
typedef floatp realp;
             ^
note: 'floatp' declared here
typedef float * restrict floatp;
                       ^

(the patch adds code that tries to look back through a chain of typedefs to find where the restrict was introduced, and then produces a corresponding series of notes, hopefully making it easy for the user to see where the restrict is coming from).

Second, we add a corresponding warning for restrict in case expressions, like this:

extern int *r, *q;
r = (int * restrict)q;

for(i=0; i<100; i++)
  *(int * restrict)p++ =   r[i];

will yield:

warning: restrict-qualified type in a cast expression has no effect [-Wignored-qualifiers]
       r = (int * restrict)q;
            ^~~~~~~~~~~~~~

warning: restrict-qualified type in a cast expression has no effect [-Wignored-qualifiers]
           *(int * restrict)p++ =   r[i];
             ^~~~~~~~~~~~~~

One remaining problem (and I'm not sure how to best solve this), the cast warning should not fire when the cast is the top-level expression on the RHS of an assignment, and the cast is making the assignment legal. Suggestions here would be greatly appreciated.

Thanks again!

Diff Detail

Event Timeline

hfinkel updated this revision to Diff 15151.Oct 20 2014, 1:05 PM
hfinkel retitled this revision from to Enhanced ignored-qualifiers checks for restrict.
hfinkel updated this object.
hfinkel edited the test plan for this revision. (Show Details)
hfinkel added a subscriber: Unknown Object (MLST).
rsmith edited edge metadata.EditedOct 20 2014, 2:17 PM

First off, more warnings for dubious uses of restrict seem valuable to me. The two warnings you suggest seem like good ideas. 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. 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).

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 *p is based on a.

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.

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.

  • Original Message -----

From: "Richard Smith" <richard@metafoo.co.uk>
To: hfinkel@anl.gov, dberlin@dberlin.org, "aaron ballman" <aaron.ballman@gmail.com>, richard@metafoo.co.uk
Cc: cfe-commits@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?

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;

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.

Thanks again,
Hal

http://reviews.llvm.org/D5872

aaron.ballman resigned from this revision.Oct 13 2015, 5:55 AM
aaron.ballman removed a reviewer: aaron.ballman.