[BoundsSafety] Delay processing of bounds attrs in templates

Dynamic bounds attributes do not handle value dependent arguments. To
enable wider interop with C++ code bases we delay the processing of
these attributes inside templated contexts. Instead we apply the new
type while instantiating the function.

One issue with this approach is that instantiation happens after parsing
is finished, and the Scope information we use to prevent attributes from
referring to arguments from outer scopes is only available during
parsing. These diagnostics were emitted at the end of the type
processing. In templated contexts we instead perform that analysis
during parsing, and delay the rest of the processing. To avoid churn in
unrelated tests the rest of the attributes keep their processing as is,
until we've investigated what impact hoisting it would have on the user
experience.

Author: hnrklssn

clang/include/clang/Sema/Sema.h

@@ -15509,7 +15509,8 @@ class Sema final : public SemaBase {
                                     AttributeCommonInfo::Kind Kind,
                                     Expr *AttrArg, SourceLocation Loc,
                                     SourceRange Range, StringRef DiagName,
-                                    bool OriginatesInAPINotes = false);
+                                    bool OriginatesInAPINotes = false,
+                                    bool InInstantiatedTemplate = false);
 
   /// Perform Bounds Safety Semantic checks for assigning to a `__counted_by` or
   /// `__counted_by_or_null` pointer type \param LHSTy.

clang/lib/Sema/DynamicCountPointerAssignmentAnalysis.cpp

@@ -3484,6 +3484,9 @@ namespace clang {
 
 void DynamicCountPointerAssignmentAnalysis::run() {
   AnalysisDeclContext AC(/* AnalysisDeclContextManager */ nullptr, dcl);
+  if (isa<TemplateDecl>(dcl)) // delay processing until template has been
+                              // instantiated
+    return;
 
   CFG *cfg = AC.getCFG();
   if (!cfg)

clang/lib/Sema/SemaDeclAttr.cpp

@@ -10,6 +10,8 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "DynamicCountPointerAssignmentAnalysis.h"
+#include "TreeTransform.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTMutationListener.h"
@@ -72,8 +74,6 @@
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TargetParser/Triple.h"
-#include "TreeTransform.h"
-#include "DynamicCountPointerAssignmentAnalysis.h"
 #include <optional>
 
 using namespace clang;
@@ -6141,6 +6141,66 @@ static bool CheckArgLifetimeAndScope(
   return Invalid;
 }
 
+static BoundsAttributedType::BoundsAttrKind
+getBoundsAttrKind(AttributeCommonInfo::Kind Kind) {
+  switch (Kind) {
+  case ParsedAttr::AT_SizedBy:
+    return BoundsAttributedType::SizedBy;
+  case ParsedAttr::AT_SizedByOrNull:
+    return BoundsAttributedType::SizedByOrNull;
+  case ParsedAttr::AT_CountedBy:
+    return BoundsAttributedType::CountedBy;
+  case ParsedAttr::AT_CountedByOrNull:
+    return BoundsAttributedType::CountedByOrNull;
+  case ParsedAttr::AT_PtrEndedBy:
+    return BoundsAttributedType::EndedBy;
+  default:
+    llvm_unreachable("unexpected bounds attribute");
+  }
+}
+
+class EarlyLifetimeAndScopeCheck
+    : public RecursiveASTVisitor<EarlyLifetimeAndScopeCheck> {
+  Sema &SemaRef;
+  bool ScopeCheck;
+  Sema::LifetimeCheckKind LifetimeCheck;
+  BoundsAttributedType::BoundsAttrKind AttrKind;
+  bool IsArrayType;
+  bool HadError = false;
+  llvm::SmallPtrSet<Decl *, 16> Visited;
+
+public:
+  EarlyLifetimeAndScopeCheck(Sema &S, bool SC, Sema::LifetimeCheckKind LC,
+                             AttributeCommonInfo::Kind AK, bool IsArray)
+      : SemaRef(S), ScopeCheck(SC), LifetimeCheck(LC),
+        AttrKind(getBoundsAttrKind(AK)), IsArrayType(IsArray) {}
+
+  bool VisitDeclRefExpr(DeclRefExpr *E) {
+    ValueDecl *VD = E->getDecl();
+    bool IsNewVD = Visited.insert(VD).second;
+    if (IsNewVD) {
+      HadError |=
+          CheckArgLifetimeAndScope(SemaRef, VD, E->getExprLoc(), ScopeCheck,
+                                   LifetimeCheck, AttrKind, IsArrayType);
+    }
+    return true;
+  }
+
+  bool VisitMemberExpr(MemberExpr *E) {
+    TraverseStmt(E->getBase());
+    ValueDecl *VD = E->getMemberDecl();
+    bool IsNewVD = Visited.insert(VD).second;
+    if (IsNewVD) {
+      HadError |=
+          CheckArgLifetimeAndScope(SemaRef, VD, E->getExprLoc(), ScopeCheck,
+                                   LifetimeCheck, AttrKind, IsArrayType);
+    }
+    return true;
+  }
+
+  bool hadError() { return HadError; }
+};
+
 static bool diagnoseBoundsAttrLifetimeAndScope(
     Sema &SemaRef, const BoundsAttributedType *Ty, bool ScopeCheck,
     Sema::LifetimeCheckKind LifetimeCheck) {
@@ -6383,11 +6443,35 @@ diagnoseRangeDependentDecls(Sema &S, const ValueDecl *TheDepender,
   return HadError;
 }
 
+class DynamicBoundsAttrInfo {
+public:
+  QualType Ty;
+  unsigned EffectiveLevel;
+  bool IsFPtr;
+
+  DynamicBoundsAttrInfo(QualType DeclTy, unsigned Level) {
+    IsFPtr = false;
+    EffectiveLevel = Level;
+    Ty = DeclTy;
+    for (unsigned i = 0; i != Level; ++i) {
+      if (!Ty->isPointerType())
+        break;
+      Ty = Ty->getPointeeType();
+      if (Ty->isFunctionType()) {
+        IsFPtr = true;
+        EffectiveLevel = Level - i - 1;
+        break;
+      }
+    }
+  }
+};
+
 void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
                                         AttributeCommonInfo::Kind Kind,
                                         Expr *AttrArg, SourceLocation Loc,
                                         SourceRange Range, StringRef DiagName,
-                                        bool OriginatesInAPINotes) {
+                                        bool OriginatesInAPINotes,
+                                        bool InInstantiatedTemplate) {
   // 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)][]'
@@ -6401,22 +6485,10 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
   auto *Var = dyn_cast<VarDecl>(D);
   QualType DeclTy = TND ? TND->getUnderlyingType() : VD->getType();
 
-  bool IsFPtr = false;
-  unsigned EffectiveLevel = Level;
-  QualType Ty = DeclTy;
-  for (unsigned i = 0; i != Level; ++i) {
-    if (!Ty->isPointerType())
-      break;
-    Ty = Ty->getPointeeType();
-    if (Ty->isFunctionType()) {
-      IsFPtr = true;
-      EffectiveLevel = Level - i - 1;
-      break;
-    }
-  }
+  DynamicBoundsAttrInfo Info(DeclTy, Level);
 
   // Don't allow typedefs with __counted_by on non-function types.
-  if (TND && (!DeclTy->isFunctionType() && !IsFPtr)) {
+  if (TND && (!DeclTy->isFunctionType() && !Info.IsFPtr)) {
     Diag(Loc, diag::err_bounds_safety_typedef_dynamic_bound) << DiagName;
     return;
   }
@@ -6490,7 +6562,7 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
       return;
     }
 
-    if (EffectiveLevel != 0 &&
+    if (Info.EffectiveLevel != 0 &&
         (!isa<ParmVarDecl>(VD) || DeclTy->isBoundsAttributedType())) {
       Diag(Loc, diag::err_bounds_safety_nested_dynamic_bound) << DiagName;
       return;
@@ -6519,14 +6591,14 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
       }
     }
 
-    if (Ty->isArrayType() && OrNull &&
-        (FD || EffectiveLevel > 0 || (Var && Var->hasExternalStorage()))) {
+    if (Info.Ty->isArrayType() && OrNull &&
+        (FD || Info.EffectiveLevel > 0 || (Var && Var->hasExternalStorage()))) {
       auto ErrDiag = Diag(Loc, diag::err_bounds_safety_nullable_fam);
       // Pointers to dynamic count types are only allowed for parameters, so any
       // FieldDecl containing a dynamic count type is a FAM. I.e. a struct field
       // with type 'int(*)[__counted_by(...)]' is not valid.
       ErrDiag << CountInBytes << /*is FAM?*/ !!FD << DiagName;
-      assert(!FD || EffectiveLevel == 0);
+      assert(!FD || Info.EffectiveLevel == 0);
 
       SourceLocation FixItLoc = getSourceManager().getExpansionLoc(Loc);
       SourceLocation EndLoc =
@@ -6538,19 +6610,19 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
       return;
     }
 
-    if (Ty->isArrayType() && EffectiveLevel > 0) {
+    if (Info.Ty->isArrayType() && Info.EffectiveLevel > 0) {
       auto ErrDiag =
           Diag(
               Loc,
               diag::
                   err_bounds_safety_unsupported_address_of_incomplete_array_type)
-          << Ty;
+          << Info.Ty;
       // apply attribute anyways to avoid too misleading follow-up diagnostics
     }
   }
 
   QualType NewDeclTy{};
-  bool ScopeCheck = (Var && Var->isLocalVarDecl()) || IsFPtr;
+  bool ScopeCheck = (Var && Var->isLocalVarDecl()) || Info.IsFPtr;
   const BoundsAttributedType *ConstructedType = nullptr;
 
   bool HadAtomicError = false;
@@ -6598,19 +6670,24 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
   // nested type, so `ConstructedType` can be different from `NewDeclTy`.
   assert(ConstructedType);
 
-  auto LifetimeCheck =
-      IsFPtr ? Sema::LifetimeCheckKind::None : Sema::getLifetimeCheckKind(Var);
-  if (diagnoseBoundsAttrLifetimeAndScope(*this, ConstructedType, ScopeCheck,
+  auto LifetimeCheck = Info.IsFPtr ? Sema::LifetimeCheckKind::None
+                                   : Sema::getLifetimeCheckKind(Var);
+  // Scope information is not available after template instantiation, so this
+  // check has been performed earlier if this is a template instantiation.
+  if (!InInstantiatedTemplate &&
+      diagnoseBoundsAttrLifetimeAndScope(*this, ConstructedType, ScopeCheck,
                                          LifetimeCheck))
     return;
 
   if (VD && !isa<FunctionDecl>(VD) && !HadAtomicError) {
     if (const auto *BDTy = dyn_cast<CountAttributedType>(ConstructedType)) {
-      if (!diagnoseCountDependentDecls(*this, VD, BDTy, EffectiveLevel, IsFPtr))
-        AttachDependerDeclsAttr(VD, BDTy, EffectiveLevel);
+      if (!diagnoseCountDependentDecls(*this, VD, BDTy, Info.EffectiveLevel,
+                                       Info.IsFPtr))
+        AttachDependerDeclsAttr(VD, BDTy, Info.EffectiveLevel);
     } else if (const auto *BDTy =
                    dyn_cast<DynamicRangePointerType>(ConstructedType)) {
-      diagnoseRangeDependentDecls(*this, VD, BDTy, EffectiveLevel, IsFPtr);
+      diagnoseRangeDependentDecls(*this, VD, BDTy, Info.EffectiveLevel,
+                                  Info.IsFPtr);
     } else {
       llvm_unreachable("Unexpected bounds attributed type");
     }
@@ -6635,11 +6712,54 @@ void Sema::applyPtrCountedByEndedByAttr(Decl *D, unsigned Level,
   }
 }
 
+static void attachLateInstantiatedCountedByEndedByAttr(Sema &S, Decl *D,
+                                                       const ParsedAttr &AL,
+                                                       int Level) {
+#define ADD_ATTR(Kind)                                                         \
+  case ParsedAttr::AT_##Kind:                                                  \
+    D->addAttr(::new (S.Context)                                               \
+                   Kind##Attr(S.Context, AL, AL.getArgAsExpr(0), Level));      \
+    break;
+
+  switch (AL.getKind()) {
+    ADD_ATTR(SizedBy)
+    ADD_ATTR(SizedByOrNull)
+    ADD_ATTR(CountedBy)
+    ADD_ATTR(CountedByOrNull)
+    ADD_ATTR(PtrEndedBy)
+  default:
+    llvm_unreachable("Invalid dynamic bound attribute");
+  }
+#undef ADD_ATTR
+}
+
 static void handlePtrCountedByEndedByAttr(Sema &S, Decl *D,
                                           const ParsedAttr &AL) {
   unsigned Level;
   if (!S.checkUInt32Argument(AL, AL.getArgAsExpr(1), Level))
     return;
+
+  if (D->getDescribedTemplate() || S.CurContext->isDependentContext()) {
+    auto *TND = dyn_cast<TypedefNameDecl>(D);
+    auto *VD = dyn_cast<ValueDecl>(D);
+    auto *Var = dyn_cast<VarDecl>(D);
+    QualType DeclTy = TND ? TND->getUnderlyingType() : VD->getType();
+    DynamicBoundsAttrInfo Info(DeclTy, Level);
+    // Scope information will be invalid by the time we instantiate the
+    // template, so perform these checks now and delay the rest of the
+    // processing until the point of instantiation.
+    auto LifetimeCheck = Info.IsFPtr ? Sema::LifetimeCheckKind::None
+                                     : Sema::getLifetimeCheckKind(Var);
+    bool ScopeCheck = (Var && Var->isLocalVarDecl()) || Info.IsFPtr;
+    EarlyLifetimeAndScopeCheck EarlyCheck(S, ScopeCheck, LifetimeCheck,
+                                          AL.getKind(), Info.Ty->isArrayType());
+    EarlyCheck.TraverseStmt(AL.getArgAsExpr(0));
+    if (EarlyCheck.hadError())
+      return;
+    attachLateInstantiatedCountedByEndedByAttr(S, D, AL, Level);
+    return;
+  }
+
   AttributeCommonInfo::Kind Kind = AL.getKind();
   const IdentifierInfo *AttrName =
       AL.printAsMacro() ? AL.getMacroIdentifier() : AL.getAttrName();

clang/lib/Sema/SemaExpr.cpp

@@ -10,8 +10,8 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "DynamicCountPointerAssignmentAnalysis.h"
 #include "CheckExprLifetime.h"
+#include "DynamicCountPointerAssignmentAnalysis.h"
 #include "TreeTransform.h"
 #include "UsedDeclVisitor.h"
 #include "clang/AST/ASTConsumer.h"
@@ -1012,7 +1012,7 @@ ExprResult Sema::DefaultLvalueConversion(Expr *E,
   // BoundsSafety: rvalue of pointers with dynamic count or range will
   // automatically form wide pointers.
   auto *DBPT = T->getAs<BoundsAttributedType>();
-  if (getLangOpts().BoundsSafety && DBPT) {
+  if (getLangOpts().BoundsSafety && DBPT && !CurContext->isDependentContext()) {
     CopyExpr Copy(*this);
     SmallVector<OpaqueValueExpr *, 2> OpaqueValues;
     // if the lvalue was a member access, it's possible its base expression
@@ -7858,7 +7858,7 @@ bool Sema::CheckDynamicCountSizeForAssignment(
 
     // The count expression might be valid, but the new value assigned may not
     // be, so check again after transforming
-    if (CountExpr->isValueDependent())
+    if (CountExpr->containsErrors())
       return false;
   }
   const auto &ReplacingValues = Transform.GetReplacingValues();
@@ -8819,7 +8819,8 @@ ExprResult Sema::BuildResolvedCallExpr(Expr *Fn, NamedDecl *NDecl,
   }
 
   /* TO_UPSTREAM(BoundsSafety) ON*/
-  if (getLangOpts().BoundsSafetyAttributes && FDecl) {
+  if (getLangOpts().BoundsSafetyAttributes && FDecl &&
+      !CurContext->isDependentContext()) {
     // FIXME: We need to support function pointers and blocks that don't have
     // function decl.
     if (!checkDynamicCountPointerAsParameter(*this, FDecl, TheCall))
@@ -25073,6 +25074,10 @@ ExprResult Sema::ActOnBoundsSafetyCall(ExprResult Call) {
   if (!CallE || CallE->containsErrors())
     return Call;
 
+  // Only process instantiated versions of templates
+  if (CurContext->isDependentContext())
+    return Call;
+
   // Bail out early if there's nothing to do. (There is something to do if
   // there's a dynamic bound pointer type in this function prototype, or if
   // it's annotated with `alloc_size`, in which case we can derive the