allow redundant bounds annotations in API notes

hnrklssn · hnrklssn · commit c2ed30cab284 · 2025-01-15T14:12:36.000-08:00
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
@@ -15514,7 +15514,8 @@ class Sema final : public SemaBase {
   void applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
                                     AttributeCommonInfo::Kind Kind,
                                     Expr *AttrArg, SourceLocation Loc,
-                                    SourceRange Range, StringRef DiagName);
+                                    SourceRange Range, StringRef DiagName,
+                                    bool OriginatesInAPINotes = false);
 
   /// Perform Bounds Safety Semantic checks for assigning to a `__counted_by` or
   /// `__counted_by_or_null` pointer type \param LHSTy.
diff --git a/clang/lib/Sema/SemaAPINotes.cpp b/clang/lib/Sema/SemaAPINotes.cpp
@@ -452,9 +452,9 @@ static void applyBoundsSafety(Sema &S, ValueDecl *D,
       break;
     }
 
-    S.applyPtrCountedByEndedByAttr(D, *Info.getLevel(), Kind, ParsedExpr.get(),
-                                   D->getLocation(), D->getSourceRange(),
-                                   AttrName);
+    S.applyPtrCountedByEndedByAttr(
+        D, *Info.getLevel(), Kind, ParsedExpr.get(), D->getLocation(),
+        D->getSourceRange(), AttrName, /* originates in API notes */ true);
   }
 }
 /* TO_UPSTREAM(BoundsSafety) OFF */
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -5411,16 +5411,17 @@ class ConstructDynamicBoundType
   const BoundsAttributedType *ConstructedType = nullptr;
   unsigned Level;
   bool ScopeCheck;
+  bool AllowRedecl;
   bool AutoPtrAttributed = false;
-  bool AllowCountRedecl = false;
   bool AtomicErrorEmitted = false;
 
 public:
   explicit ConstructDynamicBoundType(Sema &S, unsigned Level,
                                      const StringRef DiagName, Expr *ArgExpr,
-                                     SourceLocation Loc, bool ScopeCheck)
+                                     SourceLocation Loc, bool ScopeCheck,
+                                     bool AllowRedecl)
       : S(S), DiagName(DiagName), ArgExpr(ArgExpr), Loc(Loc), Level(Level),
-        ScopeCheck(ScopeCheck) {}
+        ScopeCheck(ScopeCheck), AllowRedecl(AllowRedecl) {}
 
   QualType Visit(QualType T) {
     SplitQualType SQT = T.split();
@@ -5449,7 +5450,6 @@ class ConstructDynamicBoundType
 
   QualType VisitFunctionProtoType(const FunctionProtoType *FPT) {
     // The attribute applies to the return type.
-    SaveAndRestore<bool> AllowCountRedeclLocal(AllowCountRedecl, true);
     QualType QT = Visit(FPT->getReturnType());
     if (QT.isNull())
       return QualType();
@@ -5461,7 +5461,6 @@ class ConstructDynamicBoundType
 
   QualType VisitFunctionNoProtoType(const FunctionNoProtoType *FPT) {
     // The attribute applies to the return type.
-    SaveAndRestore<bool> AllowCountRedeclLocal(AllowCountRedecl, true);
     QualType QT = Visit(FPT->getReturnType());
     if (QT.isNull())
       return QualType();
@@ -5592,8 +5591,10 @@ class ConstructCountAttributedType :
   explicit ConstructCountAttributedType(Sema &S, unsigned Level,
                                         const StringRef DiagName, Expr *ArgExpr,
                                         SourceLocation Loc, bool CountInBytes,
-                                        bool OrNull, bool ScopeCheck = false)
-      : ConstructDynamicBoundType(S, Level, DiagName, ArgExpr, Loc, ScopeCheck),
+                                        bool OrNull, bool AllowRedecl,
+                                        bool ScopeCheck = false)
+      : ConstructDynamicBoundType(S, Level, DiagName, ArgExpr, Loc, ScopeCheck,
+                                  AllowRedecl),
         CountInBytes(CountInBytes), OrNull(OrNull) {
     if (!ArgExpr->getType()->isIntegralOrEnumerationType()) {
       S.Diag(Loc, diag::err_attribute_argument_type_for_bounds_safety_count)
@@ -5661,7 +5662,7 @@ class ConstructCountAttributedType :
   }
 
   QualType DiagnoseConflictingType(const CountAttributedType *T) {
-    if (AllowCountRedecl) {
+    if (AllowRedecl) {
       QualType NewTy = BuildDynamicBoundType(T->desugar());
       const auto *NewDCPTy = NewTy->getAs<CountAttributedType>();
       // We don't have a way to distinguish if '__counted_by' is conflicting or has been
@@ -5684,6 +5685,16 @@ class ConstructCountAttributedType :
     return QualType();
   }
 
+  QualType VisitFunctionProtoType(const FunctionProtoType *FPT) {
+    SaveAndRestore<bool> AllowRedeclLocal(AllowRedecl, true);
+    return ConstructDynamicBoundType::VisitFunctionProtoType(FPT);
+  }
+
+  QualType VisitFunctionNoProtoType(const FunctionNoProtoType *FPT) {
+    SaveAndRestore<bool> AllowRedeclLocal(AllowRedecl, true);
+    return ConstructDynamicBoundType::VisitFunctionNoProtoType(FPT);
+  }
+
   QualType DiagnoseConflictingType(const DynamicRangePointerType *T) {
     S.Diag(Loc, diag::err_bounds_safety_conflicting_count_range_attributes);
     return QualType();
@@ -5858,9 +5869,10 @@ class ConstructDynamicRangePointerType :
 public:
   explicit ConstructDynamicRangePointerType(
       Sema &S, unsigned Level, const StringRef DiagName, Expr *ArgExpr,
-      SourceLocation Loc, bool ScopeCheck = false,
+      SourceLocation Loc, bool AllowRedecl, bool ScopeCheck = false,
       std::optional<TypeCoupledDeclRefInfo> StartPtrInfo = std::nullopt)
-      : ConstructDynamicBoundType(S, Level, DiagName, ArgExpr, Loc, ScopeCheck),
+      : ConstructDynamicBoundType(S, Level, DiagName, ArgExpr, Loc, ScopeCheck,
+                                  AllowRedecl),
         StartPtrInfo(StartPtrInfo) {
     assert(ArgExpr->getType()->isPointerType());
   }
@@ -5900,11 +5912,11 @@ class ConstructDynamicRangePointerType :
   }
 
   QualType VisitDynamicRangePointerType(const DynamicRangePointerType *T) {
-    if (Level == 0 && T->getEndPointer() == nullptr) {
-      // T is a started_by() pointer type.
+    if (Level == 0 && (AllowRedecl || T->getEndPointer() == nullptr)) {
+      // T could be a started_by() pointer type.
       Expr *StartPtr = T->getStartPointer();
       auto StartPtrDecls = T->getStartPtrDecls();
-      assert(StartPtr);
+      assert(StartPtr || AllowRedecl);
 
       assert(ConstructedType == nullptr);
       // Construct an ended_by() pointer type.
@@ -5917,6 +5929,20 @@ class ConstructDynamicRangePointerType :
       Expr *EndPtr = DRPT->getEndPointer();
       auto EndPtrDecls = DRPT->getEndPtrDecls();
       assert(EndPtr);
+      if (auto OldEndPtr = T->getEndPointer()) {
+        assert(AllowRedecl);
+        llvm::FoldingSetNodeID NewID;
+        llvm::FoldingSetNodeID OldID;
+        EndPtr->Profile(NewID, S.Context, /*Canonical*/ true);
+        OldEndPtr->Profile(OldID, S.Context, /*Canonical*/ true);
+
+        if (NewID != OldID) {
+          S.Diag(Loc, diag::err_bounds_safety_conflicting_pointer_attributes)
+              << /* pointer */ 1 << /* end */ 3;
+          ConstructedType = nullptr;
+          return QualType();
+        }
+      }
 
       // ConstructType was already set while visiting the nested PointerType.
       // Reconstruct DRPT by merging started_by and ended_by.
@@ -6383,7 +6409,8 @@ diagnoseRangeDependentDecls(Sema &S, const ValueDecl *TheDepender,
 void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
                                         AttributeCommonInfo::Kind Kind,
                                         Expr *AttrArg, SourceLocation Loc,
-                                        SourceRange Range, StringRef DiagName) {
+                                        SourceRange Range, StringRef DiagName,
+                                        bool OriginatesInAPINotes) {
   // If the decl is invalid, the indirection Level might not exist in the type,
   // since the type may have not been constructed correctly. Example:
   // 'int (*param)[__counted_by_or_null(10)][]'
@@ -6571,13 +6598,15 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
     }
 
     auto TypeConstructor = ConstructDynamicRangePointerType(
-        *this, Level, DiagName, AttrArg, Loc, ScopeCheck, StartPtrInfo);
+        *this, Level, DiagName, AttrArg, Loc, OriginatesInAPINotes, ScopeCheck,
+        StartPtrInfo);
     NewDeclTy = TypeConstructor.Visit(DeclTy);
     HadAtomicError = TypeConstructor.hadAtomicError();
     ConstructedType = TypeConstructor.getConstructedType();
   } else {
     auto TypeConstructor = ConstructCountAttributedType(
-        *this, Level, DiagName, AttrArg, Loc, CountInBytes, OrNull, ScopeCheck);
+        *this, Level, DiagName, AttrArg, Loc, CountInBytes, OrNull,
+        OriginatesInAPINotes, ScopeCheck);
     NewDeclTy = TypeConstructor.Visit(DeclTy);
     HadAtomicError = TypeConstructor.hadAtomicError();
     ConstructedType = TypeConstructor.getConstructedType();
diff --git a/clang/test/APINotes/Inputs/Headers/BoundsUnsafe.apinotes b/clang/test/APINotes/Inputs/Headers/BoundsUnsafe.apinotes
@@ -78,8 +78,58 @@ Functions:
         BoundsSafety:
             Kind: counted_by
             BoundedBy: len
+  - Name:              asdf_counted_redundant
+    Parameters:
+      - Position:      0
+        BoundsSafety:
+            Kind: counted_by
+            BoundedBy: len
+  - Name:              asdf_ended_chained
+    Parameters:
+      - Position:      0
+        BoundsSafety:
+            Kind: ended_by
+            Level: 0
+            BoundedBy: mid
+      - Position:      1
+        BoundsSafety:
+            Kind: ended_by
+            Level: 0
+            BoundedBy: end
+  - Name:              asdf_ended_chained_reverse
+    Parameters:
+      - Position:      1
+        BoundsSafety:
+            Kind: ended_by
+            Level: 0
+            BoundedBy: end
+      - Position:      0
+        BoundsSafety:
+            Kind: ended_by
+            Level: 0
+            BoundedBy: mid
+  - Name:              asdf_ended_already_started
+    Parameters:
+      - Position:      1
+        BoundsSafety:
+            Kind: ended_by
+            Level: 0
+            BoundedBy: end
+  - Name:              asdf_ended_already_ended
+    Parameters:
+      - Position:      0
+        BoundsSafety:
+            Kind: ended_by
+            Level: 0
+            BoundedBy: mid
+  - Name:              asdf_ended_redundant
+    Parameters:
+      - Position:      0
+        BoundsSafety:
+            Kind: ended_by
+            Level: 0
+            BoundedBy: end
   - Name:              asdf_nterm
     Parameters:
       - Position:      0
         Type: "int * __attribute__((__terminated_by__(0)))"
-
diff --git a/clang/test/APINotes/Inputs/Headers/BoundsUnsafe.h b/clang/test/APINotes/Inputs/Headers/BoundsUnsafe.h
@@ -10,5 +10,12 @@ void asdf_counted_const(int * buf);
 void asdf_counted_nullable(int len, int * _Nullable buf);
 void asdf_counted_noescape(int * buf, int len);
 void asdf_counted_default_level(int * buf, int len);
+void asdf_counted_redundant(int * __attribute__((__counted_by__(len))) buf, int len);
+
+void asdf_ended_chained(int * buf, int * mid, int * end);
+void asdf_ended_chained_reverse(int * buf, int * mid, int * end);
+void asdf_ended_already_started(int * __attribute__((__ended_by__(mid))) buf, int * mid, int * end);
+void asdf_ended_already_ended(int * buf, int * mid __attribute__((__ended_by__(end))), int * end);
+void asdf_ended_redundant(int * __attribute__((__ended_by__(end))) buf, int * end);
 
 void asdf_nterm(char * buf);
diff --git a/clang/test/APINotes/boundssafety-errors.c b/clang/test/APINotes/boundssafety-errors.c
@@ -55,6 +55,20 @@ Name: OOBLevel
             Kind: counted_by
             Level: 0
             BoundedBy: static_len
+  - Name:              mismatching_count
+    Parameters:
+      - Position:      0
+        BoundsSafety:
+            Kind: counted_by
+            Level: 0
+            BoundedBy: apinote_len
+  - Name:              mismatching_end
+    Parameters:
+      - Position:      0
+        BoundsSafety:
+            Kind: ended_by
+            Level: 0
+            BoundedBy: apinote_end
 //--- SemaErrors.h
 // CHECK: SemaErrors.h:{{.*}}:{{.*}}: error: __counted_by attribute only applies to pointer arguments
 // CHECK-NEXT: oob_level
@@ -73,6 +87,12 @@ void wrong_name(int * buf, int len2);
 // CHECK-NEXT: wrong_scope
 int static_len = 5;
 void wrong_scope(int * buf);
+// CHECK: SemaErrors.h:{{.*}}:{{.*}}: error: pointer cannot have more than one count attribute
+// CHECK-NEXT: mismatching_count
+void mismatching_count(int * __attribute__((__counted_by__(header_len))) buf, int apinote_len, int header_len);
+// CHECK: SemaErrors.h:{{.*}}:{{.*}}: error: pointer cannot have more than one end attribute
+// CHECK-NEXT: mismatching_end
+void mismatching_end(int * __attribute__((__ended_by__(header_end))) buf, int * apinote_end, int * header_end);
 // CHECK: SemaErrors.c:{{.*}}:{{.*}}: fatal error: could not build module 'SemaErrors'
 
 
diff --git a/clang/test/APINotes/boundssafety.c b/clang/test/APINotes/boundssafety.c
@@ -15,7 +15,7 @@
 // CHECK: asdf_sized_n 'void (int * __sized_by_or_null(size), int)'
 // CHECK: buf 'int * __sized_by_or_null(size)':'int *'
 
-// CHECK: asdf_ended  'void (int * __ended_by(end), int * /* __started_by(buf) */ )'
+// CHECK: asdf_ended 'void (int * __ended_by(end), int * /* __started_by(buf) */ )'
 // CHECK: buf 'int * __ended_by(end)':'int *'
 // CHECK: end 'int * /* __started_by(buf) */ ':'int *'
 
@@ -38,6 +38,32 @@
 // CHECK: asdf_counted_default_level 'void (int * __counted_by(len), int)'
 // CHECK: buf 'int * __counted_by(len)':'int *'
 
+// CHECK: asdf_counted_redundant 'void (int * __counted_by(len), int)'
+// CHECK: buf 'int * __counted_by(len)':'int *'
+
+// CHECK: asdf_ended_chained 'void (int * __ended_by(mid), int * __ended_by(end) /* __started_by(buf) */ , int * /* __started_by(mid) */ )'
+// CHECK: buf 'int * __ended_by(mid)':'int *'
+// CHECK: mid 'int * __ended_by(end) /* __started_by(buf) */ ':'int *'
+// CHECK: end 'int * /* __started_by(mid) */ ':'int *'
+
+// CHECK: asdf_ended_chained_reverse 'void (int * __ended_by(mid), int * __ended_by(end) /* __started_by(buf) */ , int * /* __started_by(mid) */ )'
+// CHECK: buf 'int * __ended_by(mid)':'int *'
+// CHECK: mid 'int * __ended_by(end) /* __started_by(buf) */ ':'int *'
+// CHECK: end 'int * /* __started_by(mid) */ ':'int *'
+
+// CHECK: asdf_ended_already_started 'void (int * __ended_by(mid), int * __ended_by(end) /* __started_by(buf) */ , int * /* __started_by(mid) */ )'
+// CHECK: buf 'int * __ended_by(mid)':'int *'
+// CHECK: mid 'int * __ended_by(end) /* __started_by(buf) */ ':'int *'
+// CHECK: end 'int * /* __started_by(mid) */ ':'int *'
+
+// CHECK: asdf_ended_already_ended 'void (int * __ended_by(mid), int * __ended_by(end) /* __started_by(buf) */ , int * /* __started_by(mid) */ )'
+// CHECK: buf 'int * __ended_by(mid)':'int *'
+// CHECK: mid 'int * __ended_by(end) /* __started_by(buf) */ ':'int *'
+// CHECK: end 'int * /* __started_by(mid) */ ':'int *'
+
+// CHECK: asdf_ended_redundant 'void (int * __ended_by(end), int * /* __started_by(buf) */ )'
+// CHECK: buf 'int * __ended_by(end)':'int *'
+// CHECK: end 'int * /* __started_by(buf) */ ':'int *'
+
 // CHECK: asdf_nterm 'void (int * __terminated_by(0))'
 // CHECK: buf 'int * __terminated_by(0)':'int *'
-

Original file line number	Diff line number	Diff line change
`@@ -452,9 +452,9 @@ static void applyBoundsSafety(Sema &S, ValueDecl *D,`
`452`	`452`	`break;`
`453`	`453`	`}`
`454`	`454`
`455`		`- S.applyPtrCountedByEndedByAttr(D, *Info.getLevel(), Kind, ParsedExpr.get(),`
`456`		`- D->getLocation(), D->getSourceRange(),`
`457`		`- AttrName);`
	`455`	`+ S.applyPtrCountedByEndedByAttr(`
	`456`	`+ D, *Info.getLevel(), Kind, ParsedExpr.get(), D->getLocation(),`
	`457`	`+ D->getSourceRange(), AttrName, /* originates in API notes */ true);`
`458`	`458`	`}`
`459`	`459`	`}`
`460`	`460`	`/* TO_UPSTREAM(BoundsSafety) OFF */`