warning for restrict-qualified pointer assignments with undefined behavior

[hfinkel]

Summary

This patch introduces a warning for undefined behavior introduced by assignments to a restrict-qualified pointer an expression based on other restrict qualified pointers. Roughly speaking, this happens when the LHS is designated by an identifier with a scope contained by the scope of the RHS designator (you can't assign from an "inner" restrict pointer to an "outer" one -- or between two at the same level).

For example, if we have:

struct S {
  int * restrict x;
  int * restrict y;
};

void f6(int * restrict q1, int * restrict q2, struct S s, struct S *t)
{
  s.x = q1;
  t->y = s.x;
}

you'll get these warnings (with some hopefully-useful notes):

warning: assignment to a restrict-qualified pointer designated by 's' using an expression based on a restrict-qualified
      pointer designated by 'q1' has undefined behavior [-Wundefined-restrict-assignment]
  s.x = q1;
      ^
note: the block associated with 'q1', declared here
void f6(int * restrict q1, int * restrict q2, struct S s, struct S *t)
                       ^
note: does not begin before the block associated with 's', declared here
void f6(int * restrict q1, int * restrict q2, struct S s, struct S *t)
                                                       ^
warning: assignment to a restrict-qualified pointer designated by 't' using an expression based on a restrict-qualified
      pointer designated by 's' has undefined behavior [-Wundefined-restrict-assignment]
  t->y = s.x;
       ^
note: the block associated with 's', declared here
void f6(int * restrict q1, int * restrict q2, struct S s, struct S *t)
                                                        ^
note: does not begin before the block associated with 't', declared here
void f6(int * restrict q1, int * restrict q2, struct S s, struct S *t)
                                                                    ^

Currently, this only determines the "based on" relationship via a syntactic analysis of the expression. In the future, I'd like to make this an analysis-based warning so that some data-flow information can inform the "based on" determination in case it flows through other non-restrict-qualified pointers.

Thanks again!

[rsmith]

The restriction on assigning between restrict-qualified pointers at the same level seems like a bug in the C spec to me. It makes no sense to me that

int *restrict p = /*...*/;
{
  int *restrict q = p;
}

should be fine, but removing the braces should make it UB.

lib/Sema/SemaExpr.cpp

8624

I don't think this is what you mean. Linkage is not relevant to the restrict rules as far as I can see; I think you're trying to check the storage class here. Maybe VarDecl::hasGlobalStorage() is what you want?

8626–8629

Needs more braces. =)

8634–8638

I would think we should walk up to the block of the member function for a C++ this expression. Calling a restrict-qualified member function on a non-restrict object should only apply the restriction for the scope of the function.

8640

I'd expect this assert to fire when defining a member function out of line.

8646–8647

You should only look at the chosen subexpression. Choose is supposed to be entirely transparent once its subexpression is chosen.

8651–8653

You can't assume the array operand is on the left-hand side. getBase?

8656–8659

I don't think this is right in a lot of cases. For instance, you walk from a CallExpr to its arguments. Since you're going to tell the user their code has undefined behavior, you should err towards false negatives.

8682–8683

Weird indentation (here and elsewhere). Tabs?

8811–8812

You're using scopes pretty heavily here, but this code can be reached from template instantiation, where there are no scopes (and because restrict is part of a type, it's possible for the restrictness to not be known until instantiation). Maybe the easiest thing to do for now is to just skip all of this if we're performing template instantiation.

This stuff also won't work as-written with dependent types and dependent expressions.

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_assignment_undefined : Warning<
+  "assignment to a restrict-qualified pointer designated by '%0' using an "
+  "expression based on a restrict-qualified pointer designated by '%1' has "
+  "undefined behavior">,
+  InGroup<DiagGroup<"undefined-restrict-assignment">>;
+def note_restrict_assignment_undefined_block_rhs : Note<
+  "the block associated with '%0', declared here">;
+def note_restrict_assignment_undefined_block_lhs : Note<
+  "does not begin before the block associated with '%0', declared here">;
+
 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:
   /// This routine must only be used when it is certain that semantic analysis
   /// and the parser are in precisely the same context, which is not the case
   /// when, e.g., we are performing any kind of template instantiation.
   /// Therefore, the only safe places to use this scope are in the parser
   /// itself and in routines directly invoked from the parser and *never* from
   /// template substitution or instantiation.
   Scope *getCurScope() const { return CurScope; }
 
+  /// \brief Retrieve the parser's translation-unit scope.
+  Scope *getTUScope() const { return TUScope; }
+
   void incrementMSLocalManglingNumber() const {
     return CurScope->incrementMSLocalManglingNumber();
   }
 
   IdentifierInfo *getSuperIdentifier() const;
   IdentifierInfo *getFloat128Identifier() const;
 
   Decl *getObjCDeclContext() const;

lib/Sema/SemaExpr.cpp

static void CheckIdentityFieldAssignment(Expr *LHSExpr, Expr *RHSExpr,
   if (OL && OR && OL->getDecl() == OR->getDecl()) {
     DeclRefExpr *RL = dyn_cast<DeclRefExpr>(OL->getBase()->IgnoreImpCasts());
     DeclRefExpr *RR = dyn_cast<DeclRefExpr>(OR->getBase()->IgnoreImpCasts());
     if (RL && RR && RL->getDecl() == RR->getDecl())
       Sema.Diag(Loc, diag::warn_identity_field_assign) << 1;
   }
 }
 
+namespace {
+  /// A visitor to extract the ordinary identifier (and scope) used to
+  /// designate a restrict-qualified pointer.
+  struct FindRestrictScope : StmtVisitor<FindRestrictScope> {
+    Sema &S;
+    SmallVector<std::pair<DeclRefExpr *, Scope *>, 4> Vars;
+
+    FindRestrictScope(Sema &S) : S(S) {}
+
+    void VisitDeclRefExpr(DeclRefExpr *E) {
+      if (E->getDecl()->hasExternalFormalLinkage())
+        Vars.push_back(std::make_pair(E, S.getTUScope()));
+      else
+        for (Scope *DS = S.getCurScope(); DS; DS = DS->getParent())
+          if (DS->isDeclScope(E->getDecl()))
+            Vars.push_back(std::make_pair(E, DS));
+    }
+
+    void VisitCXXThisExpr(CXXThisExpr *E) {
+      Scope *CS = nullptr;
+      for (Scope *SS = S.getCurScope(); SS; SS = SS->getParent())
+        if (SS->isClassScope()) {
+          CS = SS;
+          break;
+        }
+
+      assert(CS && "No class scope with a 'this' expression?");
+      Vars.push_back(std::make_pair((DeclRefExpr *) nullptr, CS));
+    }
+
+    void VisitChooseExpr(ChooseExpr *E) {
+      // The condition expression cannot contain the designating identifier.
+      Visit(E->getLHS());
+      Visit(E->getRHS());
+    }
+
+    void VisitArraySubscriptExpr(ArraySubscriptExpr *E) {
+      // The array subscript expression cannot contain the designating
+      // identifier.
+      Visit(E->getLHS());
+    }
+
+    void VisitStmt(Stmt *S) {
+      for (Stmt::child_range C = S->children(); C; ++C)
+        if (*C)
+          Visit(*C);
+    }
+  };
+
+  struct FindRestrictBasedOnScope : StmtVisitor<FindRestrictBasedOnScope> {
+    Sema &S;
+    SmallVector<std::pair<DeclRefExpr *, Scope *>, 4> Vars;
+
+    FindRestrictBasedOnScope(Sema &S) : S(S) {}
+
+    void VisitDeclRefExpr(DeclRefExpr *E) {
+      if (E->getType().isRestrictQualified()) {
+        if (E->getDecl()->hasExternalFormalLinkage())
+          Vars.push_back(std::make_pair(E, S.getTUScope()));
+        else
+          for (Scope *DS = S.getCurScope(); DS; DS = DS->getParent())
+            if (DS->isDeclScope(E->getDecl()))
+              Vars.push_back(std::make_pair(E, DS));
+      }
+    }
+
+    void VisitMemberExpr(MemberExpr *E) {
+      if (E->getType().isRestrictQualified()) {
+  // This is a restrict-qualified member; to understand the scope, we
+  // need to find the designating identifier.
+        FindRestrictScope RS(S);
+        RS.Visit(E->getBase());
+        Vars.append(RS.Vars.begin(), RS.Vars.end());
+      } else {
+        return VisitStmt(E);
+      }
+    }
+
+    void VisitStmt(Stmt *S) {
+      for (Stmt::child_range C = S->children(); C; ++C)
+        if (*C)
+          Visit(*C);
+    }
+  };
+}
+
 // C99 6.5.16.1
 QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
                                        SourceLocation Loc,
                                        QualType CompoundType) {
   assert(!LHSExpr->hasPlaceholderType(BuiltinType::PseudoObject));
 
   // Verify that LHS is a modifiable lvalue, and emit error if not.
   if (CheckForModifiableLvalue(LHSExpr, Loc, *this))
QualType Sema::CheckAssignmentOperands(Expr *LHSExpr, ExprResult &RHS,
   }
 
   if (DiagnoseAssignmentResult(ConvTy, Loc, LHSType, RHSType,
                                RHS.get(), AA_Assigning))
     return QualType();
 
   CheckForNullPointerDereference(*this, LHSExpr);
 
+  // C99 6.7.3.1p4: If P is assigned the value of a pointer expression E that
+  // is based on another restricted pointer object P2, associated with block
+  // B2, then either the execution of B2 shall begin before the execution of B,
+  // or the execution of B2 shall end prior to the assignment. If these
+  // requirements are not met, then the behavior is undefined.
+  //
+  // where the block associated with a restrict-qualified pointer is defined by:
+  //
+  // C99 6.7.3.1p1: Let D be a declaration of an ordinary identifier that
+  // provides a means of designating an object P as a restrict-qualified pointer
+  // to type T.
+  // C99 6.7.3.1p2: If D appears inside a block and does not have storage
+  // class extern, let B denote the block. If D appears in the list of
+  // parameter declarations of a function definition, let B denote the
+  // associated block. Otherwise, let B denote the block of main (or the block
+  // of whatever function is called at program startup in a freestanding
+  // environment).
+  if (CompoundType.isNull() && LHSType.isRestrictQualified()) {
+    FindRestrictScope LHSRS(*this);
+    FindRestrictBasedOnScope RHSRS(*this);
+
+    LHSRS.Visit(LHSExpr);
+    RHSRS.Visit(RHS.get());
+
+    for (auto &L : LHSRS.Vars) {
+      for (auto &R : RHSRS.Vars) {
+        // The the LHS scope is a parent of (or equal to) the RHS scope, then
+  // produce a warning.
+        //
+  // This check specifically includes, as undefined, the case where the
+  // LHS is in class scope and the RHS in in function scope because class
+  // scope objects are designated by the implicit 'this' function
+  // parameter, which is taken to have function scope, making an
+  // assignment between a class-scope restrict-qualified
+  // pointer based on a function scope restrict-qualified pointer
+  // undefined.
+
+  // class scope is equivalent to function scope (because member
+  // variables are designated by 'this', which is an implicit function
+  // parameter).
+        Scope *SS = R.second;
+        if (SS->isClassScope() && CurScope->getFnParent())
+          SS = CurScope->getFnParent();
+
+        for (Scope *S = SS; S; S = S->getParent())
+          if (L.second == S) {
+            if (L.second->isClassScope() && R.second->isClassScope()) {
+              Diag(Loc, diag::warn_restrict_assignment_undefined)
+                << "this" << "this";
+            } else if (L.second->isClassScope()) {
+              Diag(Loc, diag::warn_restrict_assignment_undefined)
+                << "this" << R.first;
+              Diag(R.first->getDecl()->getLocation(),
+                   diag::note_restrict_assignment_undefined_block_rhs)
+                << R.first;
+              Diag(getCurFunctionDecl()->getLocation(),
+                   diag::note_restrict_assignment_undefined_block_lhs)
+                << "this";
+            } else if (R.second->isClassScope()) {
+              Diag(Loc, diag::warn_restrict_assignment_undefined)
+                << L.first << "this";
+              Diag(getCurFunctionDecl()->getLocation(),
+                   diag::note_restrict_assignment_undefined_block_rhs)
+                << "this";
+              Diag(L.first->getDecl()->getLocation(),
+                   diag::note_restrict_assignment_undefined_block_lhs)
+                << L.first;
+            } else {
+              Diag(Loc, diag::warn_restrict_assignment_undefined)
+                << L.first << R.first;
+              if (R.first->getDecl() != L.first->getDecl()) {
+                Diag(R.first->getDecl()->getLocation(),
+                     diag::note_restrict_assignment_undefined_block_rhs)
+                  << R.first;
+                Diag(L.first->getDecl()->getLocation(),
+                     diag::note_restrict_assignment_undefined_block_lhs)
+                  << L.first;
+              }
+            }
+            break;
+          }
+      }
+    }
+  }
+
   // C99 6.5.16p3: The type of an assignment expression is the type of the
   // left operand unless the left operand has qualified type, in which case
   // it is the unqualified version of the type of the left operand.
   // C99 6.5.16.1p2: In simple assignment, the value of the right operand
   // is converted to the type of the assignment expression (above).
   // C++ 5.17p1: the type of the assignment expression is that of its left
   // operand.
   return (getLangOpts().CPlusPlus

test/Sema/restrict-assignment.c

+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -fsyntax-only -Wignored-qualifiers -verify
+
+// expected-note@+6 {{does not begin before the block associated with 'p1', declared here}}
+// expected-note@+5 {{does not begin before the block associated with 'p1', declared here}}
+// expected-note@+4 {{the block associated with 'p2', declared here}}
+// expected-note@+3 {{does not begin before the block associated with 'p1', declared here}}
+// expected-note@+2 {{the block associated with 'p2', declared here}}
+// expected-note@+1 {{does not begin before the block associated with 'p1', declared here}}
+int * restrict p1, * restrict p2;
+
+struct S {
+  int * restrict x;
+  int * restrict y;
+};
+
+// expected-note@+9 {{the block associated with 'q1', declared here}}
+// expected-note@+8 {{the block associated with 'q2', declared here}}
+// expected-note@+7 {{does not begin before the block associated with 'q1', declared here}}
+// expected-note@+6 {{the block associated with 'q1', declared here}}
+// expected-note@+5 {{does not begin before the block associated with 's', declared here}}
+// expected-note@+4 {{the block associated with 's', declared here}}
+// expected-note@+3 {{does not begin before the block associated with 't', declared here}}
+// expected-note@+2 {{does not begin before the block associated with 'q1', declared here}}
+// expected-note@+1 {{the block associated with 'q2', declared here}}
+void foo(int * restrict q1, int * restrict q2, struct S s, struct S *t)
+{
+  // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'p1' using an expression based on a restrict-qualified pointer designated by 'p2' has undefined behavior}}
+  p1 = p2;
+  // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'p1' using an expression based on a restrict-qualified pointer designated by 'p2' has undefined behavior}}
+  p1 = p2+1;
+
+  // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'p1' using an expression based on a restrict-qualified pointer designated by 'q1' has undefined behavior}}
+  p1 = q1;
+  // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'q1' using an expression based on a restrict-qualified pointer designated by 'q2' has undefined behavior}}
+  q1 = q2;
+
+  // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 's' using an expression based on a restrict-qualified pointer designated by 'q1' has undefined behavior}}
+  s.x = q1;
+  // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 't' using an expression based on a restrict-qualified pointer designated by 's' has undefined behavior}}
+  t->y = s.x;
+  {
+    // expected-note@+4 {{the block associated with 'r2', declared here}}
+    // expected-note@+3 {{does not begin before the block associated with 'r1', declared here}}
+    // expected-note@+2 {{the block associated with 'r1', declared here}}
+    // expected-note@+1 {{the block associated with 'r1', declared here}}
+    int * restrict r1, * restrict r2;
+    // expected-note@+1 {{does not begin before the block associated with 'er1', declared here}}
+    extern int * restrict er1;
+
+    // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'r1' using an expression based on a restrict-qualified pointer designated by 'r2' has undefined behavior}}
+    r1 = r2;
+    // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'q1' using an expression based on a restrict-qualified pointer designated by 'r1' has undefined behavior}}
+    q1 = r1;
+    // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'p1' using an expression based on a restrict-qualified pointer designated by 'r1' has undefined behavior}}
+    p1 = r1;
+
+    r1 = p2; // ok
+    r1 = q2; // ok
+    r2 = s.y; // ok
+
+    // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'er1' using an expression based on a restrict-qualified pointer designated by 'q2' has undefined behavior}}
+    er1 = q2;
+  }
+}
+

test/SemaCXX/restrict-assignment.cpp

+// RUN: %clang_cc1 %s -std=c++11 -fsyntax-only -Wignored-qualifiers -verify
+
+class C {
+  // expected-note@+9 {{the block associated with 'x', declared here}}
+  // expected-note@+8 {{does not begin before the block associated with 'this', declared here}}
+  // expected-note@+7 {{the block associated with 'x', declared here}}
+  // expected-note@+6 {{the block associated with 'this', declared here}}
+  // expected-note@+5 {{does not begin before the block associated with 'this', declared here}}
+  // expected-note@+4 {{the block associated with 'x', declared here}}
+  // expected-note@+3 {{does not begin before the block associated with 'this', declared here}}
+  // expected-note@+2 {{the block associated with 'this', declared here}}
+  // expected-note@+1 {{does not begin before the block associated with 'x', declared here}}
+  void foo(float *__restrict__ x) {
+    // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'this' using an expression based on a restrict-qualified pointer designated by 'x' has undefined behavior}}
+    a = x;
+    // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'this' using an expression based on a restrict-qualified pointer designated by 'this' has undefined behavior}}
+    a = b;
+
+    // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'sa' using an expression based on a restrict-qualified pointer designated by 'x' has undefined behavior}}
+    sa = x;
+    // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'sa' using an expression based on a restrict-qualified pointer designated by 'this' has undefined behavior}}
+    sa = a;
+
+    {
+      // expected-note@+2 {{the block associated with 'q', declared here}}
+      // expected-note@+1 {{the block associated with 'q', declared here}}
+      float * __restrict__ q;
+
+      q = sa; // ok
+      q = a;  // ok
+
+      // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'sa' using an expression based on a restrict-qualified pointer designated by 'q' has undefined behavior}}
+      sa = q;
+      // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'this' using an expression based on a restrict-qualified pointer designated by 'q' has undefined behavior}}
+      b = q;
+    }
+
+    // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'this' using an expression based on a restrict-qualified pointer designated by 'this' has undefined behavior}}
+    s.a = a;
+    // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'this' using an expression based on a restrict-qualified pointer designated by 'this' has undefined behavior}}
+    a = s.a;
+
+    // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'this' using an expression based on a restrict-qualified pointer designated by 'x' has undefined behavior}}
+    s.a = x;
+    // expected-warning@+1 {{assignment to a restrict-qualified pointer designated by 'x' using an expression based on a restrict-qualified pointer designated by 'this' has undefined behavior}}
+    x = s.a;
+  }
+
+  float *__restrict__ a;
+  float *__restrict__ b;
+
+  // expected-note@+3 {{does not begin before the block associated with 'sa', declared here}}
+  // expected-note@+2 {{does not begin before the block associated with 'sa', declared here}}
+  // expected-note@+1 {{does not begin before the block associated with 'sa', declared here}}
+  static float * __restrict__ sa;
+
+  struct {
+    float *__restrict__ a;
+  } s;
+};
+