Implement restrictions on taking addresses of members and variables. (#…

…490)

Checked C restricts taking the addresses of:
1. Members with member bounds declarations.
2. Members used in member bounds declarations.
3. Variables with bounds declarations.
4. Variables/variable members used in bounds declarations.

This change implements restrictions 1 through 3.  This addresses issue #213 and part of issue #212.
- Taking the address of non-array members with or used in bounds declarations is now an error. 
- Taking the address of non-array members with or used in bounds-safe interfaces is allowed in unchecked scopes.  It is an error in checked scopes.    In unchecked scopes, we don't warn about this, even though it could go wrong.  We don't want to add warnings for existing code.  We could add an optional warning if we wanted to.
- Taking the address of non-array variables with bounds declaration is now an error.

It is OK to take the address of an array variable or member because you can't use the resulting pointer to modify the pointer that the array converts to.

We recognize variations on these restrictions, such as taking the address of a parenthesized lvalue expression or taking the address of an expression where a bounds-safe interface cast has been inserted.

This was more complex to implement than the specification describes because of the possible use of nested members.   See the long comment describing the algorithm in CheckedCAlias.cpp.  We handle a few different cases:
- The simple case where a member bounds expression only uses members declared in the current structure.
- A member bounds expression uses members from a nested structure of the current structure.  In this case, we do not allow the address of the nested structure to be taken.  Given 
```
struct NestedLen {
   int len; 
};

struct S {
   struct NestedLen n;
   _Array_ptr<int> p : count(n.len);
}
```
we don't allow the address of n to be taken.
- A structure with bounds is embedded in another structure, and then an address of member used in member bounds is taken.

We handle these cases by recognizing "paths" of member accesses and using that to recognize paths that may result in aliases to members used in bounds, and paths that won't result in aliases to members used in bounds.

All of the benchmarks that we converted to Checked C compile with these restrictions in place.  This is a step toward removing one of the caveats in the SecDev paper.  We still need to restrict taking the address of variables and variable members used in bounds declarations to complete the aliasing restrictions.

 It'll be interesting to see what happens on real-world code.   We could loosen some of the restrictions, for example, and allow taking the addresses involving constant-sized bounds (bounds that only involve the variable or member).

I found and fixed a bug in the lexicographic comparison of declarations.   We were supposed to be comparing pointers to names in declarations, and compared pointers to the declarations instead.  We then tried dereferencing the pointers to names.

Testing:
- I've added a new file containing tests of address-of expressions in the Checked C repo.  This will be subject to a separate pull request.
- Passed clang tests on Linux x64.
- Passed LNT tests on Linux x64.
- Passed automated testing on Windows.

include/clang/AST/ASTContext.h

@@ -2479,7 +2479,7 @@ class ASTContext : public RefCountedBase<ASTContext> {
   void ResetObjCLayout(const ObjCContainerDecl *CD);
 
   //===--------------------------------------------------------------------===//
-  //           Predicates For Checked C checked types and bounds
+  //          Predicates and methods for Checked C checked types and bounds
   //===--------------------------------------------------------------------===//
 
   /// \brief Determine whether a pointer, array, or function type T1 provides
@@ -2526,6 +2526,52 @@ class ASTContext : public RefCountedBase<ASTContext> {
   BoundsExpr *getPrebuiltCountOne();
   BoundsExpr *getPrebuiltBoundsUnknown();
 
+  // Track the set of member bounds declarations that use a given
+  // member path.   For each member bounds declaration, we store the
+  // field with the declaration, not the member bound itself.
+
+  // Members are stored in reverse order.  Given a.b.c, we store c.b.a
+  typedef SmallVector<const FieldDecl *,4> MemberPath;
+  struct PathCompare {
+  private:
+    Lexicographic Comparer;
+  public:
+    PathCompare(ASTContext &Context) : Comparer(Lexicographic(Context, nullptr)) {}
+
+    bool operator()(const MemberPath &P1, const MemberPath &P2) const {
+      if (P1.size() < P2.size())
+        return true;
+      if (P1.size() > P2.size())
+        return false;
+      for (int i = 0; i < P1.size(); i++) {
+         Lexicographic::Result R = Comparer.CompareDecl(P1[i],P2[i]);
+         if (R == Lexicographic::Result::LessThan)
+           return true;
+         if (R == Lexicographic::Result::GreaterThan)
+           return false;
+      }
+      return false;
+    }
+  };
+
+  typedef llvm::TinyPtrVector<const FieldDecl*> MemberDeclVector;
+private:
+  std::map<MemberPath, MemberDeclVector, PathCompare> UsingBounds;
+
+public:
+  typedef MemberDeclVector::const_iterator member_bounds_iterator;
+  member_bounds_iterator using_member_bounds_begin(const MemberPath &Path) const;
+  member_bounds_iterator using_member_bounds_end(const MemberPath &Path) const;
+
+  unsigned using_member_bounds_size(const MemberPath &Path) const;
+  typedef llvm::iterator_range<member_bounds_iterator> member_bounds_iterator_range;
+  member_bounds_iterator_range using_member_bounds(const MemberPath &Path) const;
+
+  /// \brief Note that \p MemberPath is used by the member bounds for
+  /// \p UsingBounds.
+  void addMemberBoundsUse(const MemberPath &MemberPath,
+                          const FieldDecl *UsingBounds);
+
   /// \brief Given an InteropTypeExpr pointer, return the interop type.
   /// Adjust the type if the type is for a parameter.  Return a null QualType
   /// if the pointer is null.

include/clang/AST/CanonBounds.h

@@ -66,11 +66,11 @@ namespace clang {
     // they are not, return Current.
     Result CheckEquivExprs(Result Current, const Expr *E1, const Expr *E2);
 
-    Result CompareInteger(signed I1, signed I2);
-    Result CompareInteger(unsigned I1, unsigned I2);
+    Result CompareInteger(signed I1, signed I2) const;
+    Result CompareInteger(unsigned I1, unsigned I2) const;
     Result CompareRelativeBoundsClause(const RelativeBoundsClause *RC1,
                                        const RelativeBoundsClause *RC2);
-    Result CompareScope(const DeclContext *DC1, const DeclContext *DC2);
+    Result CompareScope(const DeclContext *DC1, const DeclContext *DC2) const;
 
     Result CompareImpl(const PredefinedExpr *E1, const PredefinedExpr *E2);
     Result CompareImpl(const DeclRefExpr *E1, const DeclRefExpr *E2);
@@ -112,9 +112,9 @@ namespace clang {
 
     /// \brief Compare declarations that may be used by expressions or
     /// or types.
-    Result CompareDecl(const NamedDecl *D1, const NamedDecl *D2);
-    Result CompareType(QualType T1, QualType T2);
-    Result CompareTypeIgnoreCheckedness(QualType QT1, QualType QT2);
+    Result CompareDecl(const NamedDecl *D1, const NamedDecl *D2) const;
+    Result CompareType(QualType T1, QualType T2) const;
+    Result CompareTypeIgnoreCheckedness(QualType QT1, QualType QT2) const;
   };
 }  // end namespace clang
 

include/clang/Basic/DiagnosticSemaKinds.td

@@ -9602,6 +9602,15 @@ def err_bounds_type_annotation_lost_checking : Error<
     "|argument for parameter used in function return bounds expression"
     "|argument for parameter used in function parameter bounds expression}1">;
 
+  def err_address_of_var_with_bounds : Error<"cannot take address of variable %0 with bounds">;
+
+  def err_address_of_member_in_bounds : Error<"cannot take address of member used in member bounds">;
+
+  def err_address_of_member_with_bounds : Error<"cannot take address of member with bounds">;
+
+  def note_var_bounds : Note<"bounds declared here">;
+  def note_member_bounds : Note<"member bounds declared here">;
+
   def err_checked_scope_decl_type: Error<
     "%select{parameter|return|local variable|global variable|member}0 %select{|used }1in a checked scope must have "
     "%select{a |a pointer, array or function type that uses only |a bounds-safe interface type that uses only }2"

include/clang/Sema/Sema.h

@@ -4649,6 +4649,11 @@ class Sema {
 
   bool DiagnoseBoundsDeclType(QualType Ty, DeclaratorDecl *D,
                               BoundsAnnotations &BA, bool IsReturnAnnots);
+
+  /// \\brief Update information in ASTContext tracking for a member what
+  /// bounds declarations depend upon it.  FD is the member whose
+  /// bounds are given by Bounds.
+  void TrackMemberBoundsDependences(FieldDecl *FD, BoundsExpr *Bounds);
   void ActOnBoundsDecl(DeclaratorDecl *D, BoundsAnnotations Annots,
                        bool MergeDeferredBounds = false);
 
@@ -4770,6 +4775,10 @@ class Sema {
       BDC_Initialization
   };
 
+  /// \brief Check that address=of operation is not taking the
+  /// address of members used in bounds.
+  void CheckAddressTakenMembers(UnaryOperator *AddrOf);
+
   /// CheckFunctionBodyBoundsDecls - check bounds declarations within a function
   /// body.
   void CheckFunctionBodyBoundsDecls(FunctionDecl *FD, Stmt *Body);
@@ -4779,6 +4788,7 @@ class Sema {
   void CheckTopLevelBoundsDecls(VarDecl *VD);
 
 
+
   // WarnDynamicCheckAlwaysFails - Adds a warning if an explicit dynamic check
   // will always fail.
   void WarnDynamicCheckAlwaysFails(const Expr *Condition);

lib/AST/ASTContext.cpp

@@ -760,6 +760,7 @@ ASTContext::ASTContext(LangOptions &LOpts, SourceManager &SM,
       CommentCommandTraits(BumpAlloc, LOpts.CommentOpts),
       PrebuiltByteCountOne(nullptr), PrebuiltCountZero(nullptr),
       PrebuiltCountOne(nullptr), PrebuiltBoundsUnknown(nullptr),
+      UsingBounds(PathCompare(*this)),
       LastSDM(nullptr, 0) {
   TUDecl = TranslationUnitDecl::Create(*this);
 }
@@ -8780,7 +8781,7 @@ QualType ASTContext::mergeObjCGCQualifiers(QualType LHS, QualType RHS) {
 }
 
 //===--------------------------------------------------------------------===//
-//            Predicates For Checked C checked types and bounds
+//        Predicates and methods for Checked C checked types and bounds
 //===--------------------------------------------------------------------===//
 
 static bool lessThan(bool Self, bool Other) {
@@ -9149,6 +9150,45 @@ BoundsExpr *ASTContext::getPrebuiltBoundsUnknown() {
   return ResultType;
 }
 
+//
+// Methods for tracking which member bounds declarations use a member.
+//
+
+ASTContext::member_bounds_iterator
+ASTContext::using_member_bounds_begin(const MemberPath &Path) const {
+  auto Pos = UsingBounds.find(Path);
+  if (Pos == UsingBounds.end())
+    return nullptr;
+  return Pos->second.begin();
+}
+
+ASTContext::member_bounds_iterator
+ASTContext::using_member_bounds_end(const MemberPath &Path) const {
+  auto Pos = UsingBounds.find(Path);
+  if (Pos == UsingBounds.end())
+    return nullptr;
+  return Pos->second.end();
+}
+
+unsigned
+ASTContext::using_member_bounds_size(const MemberPath &Path) const {
+  auto Pos = UsingBounds.find(Path);
+  if (Pos == UsingBounds.end())
+    return 0;
+  return Pos->second.size();
+}
+
+ASTContext::member_bounds_iterator_range
+ASTContext::using_member_bounds(const MemberPath &Path) const {
+  return member_bounds_iterator_range(using_member_bounds_begin(Path),
+                                      using_member_bounds_end(Path));
+}
+
+void ASTContext::addMemberBoundsUse(const MemberPath &Path,
+                                    const FieldDecl *Bounds) {
+  UsingBounds[Path].push_back(Bounds);
+}
+
 //===----------------------------------------------------------------------===//
 //                         Integer Predicates
 //===----------------------------------------------------------------------===//

lib/AST/CanonBounds.cpp

@@ -140,7 +140,7 @@ Lexicographic::Lexicographic(ASTContext &Ctx, EquivExprSets *EquivExprs) :
   Context(Ctx), EquivExprs(EquivExprs), Trace(false) {
 }
 
-Result Lexicographic::CompareInteger(signed I1, signed I2) {
+Result Lexicographic::CompareInteger(signed I1, signed I2) const {
   if (I1 < I2)
     return Result::LessThan;
   else if (I1 > I2)
@@ -149,7 +149,7 @@ Result Lexicographic::CompareInteger(signed I1, signed I2) {
     return Result::Equal;
 }
 
-Result Lexicographic::CompareInteger(unsigned I1, unsigned I2) {
+Result Lexicographic::CompareInteger(unsigned I1, unsigned I2) const {
   if (I1 < I2)
     return Result::LessThan;
   else if (I1 > I2)
@@ -193,7 +193,7 @@ static Result ComparePointers(T *P1, T *P2, bool &ordered) {
 }
 
 Result
-Lexicographic::CompareScope(const DeclContext *DC1, const DeclContext *DC2) {
+Lexicographic::CompareScope(const DeclContext *DC1, const DeclContext *DC2) const {
    DC1 = DC1->getPrimaryContext();
    DC2 = DC2->getPrimaryContext();
 
@@ -220,7 +220,7 @@ Lexicographic::CompareScope(const DeclContext *DC1, const DeclContext *DC2) {
 }
 
 Result
-Lexicographic::CompareDecl(const NamedDecl *D1Arg, const NamedDecl *D2Arg) {
+Lexicographic::CompareDecl(const NamedDecl *D1Arg, const NamedDecl *D2Arg) const {
   const NamedDecl *D1 = dyn_cast<NamedDecl>(D1Arg->getCanonicalDecl());
   const NamedDecl *D2 = dyn_cast<NamedDecl>(D2Arg->getCanonicalDecl());
   if (D1 == D2) 
@@ -259,14 +259,16 @@ Lexicographic::CompareDecl(const NamedDecl *D1Arg, const NamedDecl *D2Arg) {
   const IdentifierInfo *Name2 = D2->getIdentifier();
 
   bool ordered;
-  Cmp = ComparePointers(D1, D2, ordered);
+  Cmp = ComparePointers(Name1, Name2, ordered);
   if (ordered && Cmp != Result::Equal)
     return Cmp;
-  assert(Name1 && Name2);
+  assert((Name1 != nullptr) == (Name2 != nullptr));
 
-  Cmp = TranslateInt(Name1->getName().compare(Name2->getName()));
-  if (Cmp != Result::Equal)
-    return Cmp;
+  if (Name1) {
+    Cmp = TranslateInt(Name1->getName().compare(Name2->getName()));
+    if (Cmp != Result::Equal)
+      return Cmp;
+  }
 
   // They are canonical declarations that have the same kind and the same name, but are not 
   // the same declaration.
@@ -494,7 +496,7 @@ Result Lexicographic::CheckEquivExprs(Result Current, const Expr *E1, const Expr
 
 
 Result
-Lexicographic::CompareType(QualType QT1, QualType QT2) {
+Lexicographic::CompareType(QualType QT1, QualType QT2) const {
   QT1 = QT1.getCanonicalType();
   QT2 = QT2.getCanonicalType();
   if (QT1 == QT2)
@@ -505,7 +507,7 @@ Lexicographic::CompareType(QualType QT1, QualType QT2) {
 }
 
 Result
-Lexicographic::CompareTypeIgnoreCheckedness(QualType QT1, QualType QT2) {
+Lexicographic::CompareTypeIgnoreCheckedness(QualType QT1, QualType QT2) const {
   QT1 = QT1.getCanonicalType();
   QT2 = QT2.getCanonicalType();
   if (Context.isEqualIgnoringChecked(QT1, QT2))

lib/Sema/CMakeLists.txt

@@ -11,6 +11,7 @@ endif()
 add_clang_library(clangSema
   AnalysisBasedWarnings.cpp
   AttributeList.cpp
+  CheckedCAlias.cpp
   CheckedCInterop.cpp
   CodeCompleteConsumer.cpp
   DeclSpec.cpp