Enhanced ignored-qualifiers checks for restrict

[hfinkel]

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!

[rsmith]

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.

[hfinkel]

• Original Message -----

From: "Richard Smith" <[e-mail]>
To: [e-mail], [e-mail], "aaron ballman" <[e-mail]>, [e-mail]
Cc: [e-mail]
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

include/clang/Basic/DiagnosticSemaKinds.td

def warn_typecheck_reference_qualifiers : Warning<
   "'%0' qualifier on reference type %1 has no effect">,
   InGroup<IgnoredQualifiers>;
 def err_typecheck_invalid_restrict_not_pointer : Error<
   "restrict requires a pointer or reference (%0 is invalid)">;
 def err_typecheck_invalid_restrict_not_pointer_noarg : Error<
   "restrict requires a pointer or reference">;
 def err_typecheck_invalid_restrict_invalid_pointee : Error<
   "pointer to function type %0 may not be 'restrict' qualified">;
+def warn_restrict_without_effect : Warning<
+  "restrict-qualified %select{function return type|type in a cast expression}0 "
+  "has no effect">,
+  InGroup<IgnoredQualifiers>, DefaultIgnore;
 def ext_typecheck_zero_array_size : Extension<
   "zero size arrays are an extension">, InGroup<ZeroLengthArray>;
 def err_typecheck_zero_array_size : Error<
   "zero-length arrays are not permitted in C++">;
 def warn_typecheck_zero_static_array_size : Warning<
   "'static' has no effect on zero-length arrays">,
   InGroup<ArrayBounds>;
 def err_array_size_non_int : Error<"size of array has non-integer type %0">;

include/clang/Sema/Sema.h

public:
   QualType BuildMemberPointerType(QualType T, QualType Class,
                                   SourceLocation Loc,
                                   DeclarationName Entity);
   QualType BuildBlockPointerType(QualType T,
                                  SourceLocation Loc, DeclarationName Entity);
   QualType BuildParenType(QualType T);
   QualType BuildAtomicType(QualType T, SourceLocation Loc);
 
+  void EmitDeclaredAtNotesForRestrict(QualType UT);
   TypeSourceInfo *GetTypeForDeclarator(Declarator &D, Scope *S);
   TypeSourceInfo *GetTypeForDeclaratorCast(Declarator &D, QualType FromTy);
   TypeSourceInfo *GetTypeSourceInfoForDeclarator(Declarator &D, QualType T,
                                                TypeSourceInfo *ReturnTypeInfo);
 
   /// \brief Package the given type and TSI into a ParsedType.
   ParsedType CreateParsedType(QualType T, TypeSourceInfo *TInfo);
   DeclarationNameInfo GetNameForDeclarator(Declarator &D);

lib/Sema/SemaDecl.cpp

if (isCalleeCleanup(NewTypeInfo.getCC())) {
       // (void) parameters, so use a default-error warning in this case :-/
       int DiagID = NewTypeInfo.getCC() == CC_X86StdCall
           ? diag::warn_cconv_knr : diag::err_cconv_knr;
       Diag(NewFD->getLocation(), DiagID)
           << FunctionType::getNameForCallConv(NewTypeInfo.getCC());
     }
   }
 
+  // If the return type is restrict qualified, warn the user. We only generate
+  // the warning here when the type is restrict qualified, but not locally so
+  // (qualified via a typedef, for example), because the local case is handled
+  // earlier (in diagnoseRedundantReturnTypeQualifiers).
+  if (const FunctionType *OrigNewType = dyn_cast<FunctionType>(NewFD->getType())) {
+    QualType RT = OrigNewType->getReturnType();
+    if (RT.isRestrictQualified() && !RT.isLocalRestrictQualified()) {
+      Diag(NewFD->getReturnTypeSourceRange().getBegin(),
+           diag::warn_restrict_without_effect) << 0
+        << NewFD->getReturnTypeSourceRange();
+
+      EmitDeclaredAtNotesForRestrict(RT);
+    }
+  }
+
   if (getLangOpts().CPlusPlus) {
     // C++-specific checks.
     if (CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(NewFD)) {
       CheckConstructor(Constructor);
     } else if (CXXDestructorDecl *Destructor = 
                 dyn_cast<CXXDestructorDecl>(NewFD)) {
       CXXRecordDecl *Record = Destructor->getParent();
       QualType ClassType = Context.getTypeDeclType(Record);

lib/Sema/SemaType.cpp

if (declSpecTy->isObjCObjectType() && hasIndirection)
       return transferARCOwnershipToDeclaratorChunk(state, ownership, inner);
   } else {
     assert(chunk.Kind == DeclaratorChunk::Array ||
            chunk.Kind == DeclaratorChunk::Reference);
     return transferARCOwnershipToDeclSpec(S, declSpecTy, ownership);
   }
 }
 
+void Sema::EmitDeclaredAtNotesForRestrict(QualType UT) {
+  // It is often the case that restrict has been added via a typedef, and
+  // perhaps via a chain of typedefs. Help the user figure out where the
+  // restrict was added, if possible.
+  while (const TypedefType *TT = UT->getAs<TypedefType>()) {
+    if (TT->getDecl()->getLocation().isValid())
+      Diag(TT->getDecl()->getLocation(), diag::note_entity_declared_at)
+        << TT->getDecl();
+    else
+      break;
+
+    UT = TT->getDecl()->getUnderlyingType();
+    if (!UT.isRestrictQualified())
+      break;
+  }
+}
+
 TypeSourceInfo *Sema::GetTypeForDeclaratorCast(Declarator &D, QualType FromTy) {
   TypeProcessingState state(*this, D);
 
   TypeSourceInfo *ReturnTypeInfo = nullptr;
   QualType declSpecTy = GetDeclSpecTypeForDeclarator(state, ReturnTypeInfo);
   if (declSpecTy.isNull())
     return Context.getNullTypeSourceInfo();
 
   if (getLangOpts().ObjCAutoRefCount) {
     Qualifiers::ObjCLifetime ownership = Context.getInnerObjCOwnership(FromTy);
     if (ownership != Qualifiers::OCL_None)
       transferARCOwnership(state, declSpecTy, ownership);
   }
 
-  return GetFullTypeForDeclarator(state, declSpecTy, ReturnTypeInfo);
+  TypeSourceInfo *CastTInfo =
+    GetFullTypeForDeclarator(state, declSpecTy, ReturnTypeInfo);
+
+  // If the cast type is restrict qualified, warn the user. Casting to a
+  // restrict type has no effect. We generate the diagnostic here because
+  // we have access to the Declarator source range (which will include the
+  // restrict qualifier when present locally).
+  QualType CastType = CastTInfo->getType();
+  if (CastType.isRestrictQualified() && !FromTy.isRestrictQualified()) {
+    Diag(D.getLocStart(), diag::warn_restrict_without_effect) << 1
+      << D.getSourceRange();
+
+    EmitDeclaredAtNotesForRestrict(CastType);
+  }
+
+  return CastTInfo;
 }
 
 /// Map an AttributedType::Kind to an AttributeList::Kind.
 static AttributeList::Kind getAttrListKind(AttributedType::Kind kind) {
   switch (kind) {
   case AttributedType::attr_address_space:
     return AttributeList::AT_AddressSpace;
   case AttributedType::attr_regparm:

test/Sema/restrict.c

+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -fsyntax-only -Wignored-qualifiers -verify
+
+// expected-warning@+1 {{'restrict' type qualifier on return type has no effect}}
+int * restrict fooa(void);
+
+// expected-warning@+1 {{'restrict' type qualifier on return type has no effect}}
+int * restrict foo(void) {
+  extern int i, *p, *q, *r;
+
+  // expected-warning@+1 {{restrict-qualified type in a cast expression has no effect}}
+  r = (int * restrict)q;
+
+  for (i=0; i < 1600; i++)
+    *(int * restrict)p++ = r[i];
+  // expected-warning@-1 {{restrict-qualified type in a cast expression has no effect}}
+
+  return p;
+}
+
+typedef int * restrict intp; // expected-note {{declared here}}
+// expected-warning@+1 {{restrict-qualified function return type has no effect}}
+intp foob(void);
+
+typedef float * restrict floatp; // expected-note {{declared here}}
+typedef floatp realp; // expected-note {{declared here}}
+// expected-warning@+1 {{restrict-qualified function return type has no effect}}
+realp fooc(void);
+

test/SemaCXX/restrict.cpp

+// RUN: %clang_cc1 %s -std=c++11 -fsyntax-only -Wignored-qualifiers -verify
+
+// expected-warning@+3 {{restrict-qualified function return type has no effect}}
+// expected-warning@+2 {{restrict-qualified function return type has no effect}}
+template <typename T>
+T foo() { return T(0); }
+
+void goo() {
+// expected-note@+1 {{in instantiation of function template specialization}}
+  foo<float * __restrict>();
+}
+
+typedef int * __restrict intp;
+void hoo() {
+// expected-note@+1 {{in instantiation of function template specialization}}
+  foo<intp>();
+}
+