[clang] [BoundsSafety][Sema] Allow counted_by and counted_by_or_null on pointers where the pointee type is incomplete but potentially completable (PR #106321)

[via cfe-commits]

================
@@ -186,4 +218,216 @@ bool Sema::CheckCountedByAttrOnField(FieldDecl *FD, Expr *E, bool CountInBytes,
   return false;
 }
 
+SourceRange Sema::BoundsSafetySourceRangeFor(const CountAttributedType *CATy) {
+  // This is an approximation that's not quite right. This points to the
+  // the expression inside the attribute rather than the attribute itself.
+  //
+  // TODO: Implement logic to find the relevant TypeLoc for the attribute and
+  // get the SourceRange from that (#113582).
+  return CATy->getCountExpr()->getSourceRange();
+}
+
+static void EmitIncompleteCountedByPointeeNotes(Sema &S,
+                                                const CountAttributedType *CATy,
+                                                NamedDecl *IncompleteTyDecl,
+                                                bool NoteAttrLocation = true) {
+  assert(IncompleteTyDecl == nullptr || isa<TypeDecl>(IncompleteTyDecl));
+
+  if (NoteAttrLocation) {
+    // Note where the attribute is declared
+    auto AttrSrcRange = S.BoundsSafetySourceRangeFor(CATy);
+    S.Diag(AttrSrcRange.getBegin(), diag::note_named_attribute)
+        << CATy->getAttributeName(/*WithMacroPrefix=*/true) << AttrSrcRange;
+  }
+
+  if (!IncompleteTyDecl)
+    return;
+
+  // If there's an associated forward declaration display it to emphasize
+  // why the type is incomplete (all we have is a forward declaration).
+
+  // Note the `IncompleteTyDecl` type is the underlying type which might not
+  // be the same as `CATy->getPointeeType()` which could be a typedef.
+  //
+  // The diagnostic printed will be at the location of the underlying type but
+  // the diagnostic text will print the type of `CATy->getPointeeType()` which
+  // could be a typedef name rather than the underlying type. This is ok
+  // though because the diagnostic will print the underlying type name too.
+  // E.g:
+  //
+  // `forward declaration of 'Incomplete_Struct_t'
+  //  (aka 'struct IncompleteStructTy')`
+  //
+  // If this ends up being confusing we could emit a second diagnostic (one
+  // explaining where the typedef is) but that seems overly verbose.
+
+  S.Diag(IncompleteTyDecl->getBeginLoc(), diag::note_forward_declaration)
+      << CATy->getPointeeType();
+}
+
+static bool
+HasCountedByAttrOnIncompletePointee(QualType Ty, NamedDecl **ND,
+                                    const CountAttributedType **CATyOut,
+                                    QualType *PointeeTyOut) {
+  auto *CATy = Ty->getAs<CountAttributedType>();
+  if (!CATy)
+    return false;
+
+  // Incomplete pointee type is only a problem for
+  // counted_by/counted_by_or_null
+  if (CATy->isCountInBytes())
+    return false;
+
+  auto PointeeTy = CATy->getPointeeType();
+  if (PointeeTy.isNull())
+    return false; // Reachable?
+
+  if (!PointeeTy->isIncompleteType(ND))
+    return false;
+
+  if (CATyOut)
+    *CATyOut = CATy;
+  if (PointeeTyOut)
+    *PointeeTyOut = PointeeTy;
+  return true;
+}
+
+/// Perform Checks for assigning to a `__counted_by` or
+/// `__counted_by_or_null` pointer type \param LHSTy where the pointee type
+/// is incomplete which is invalid.
+///
+/// \param S The Sema instance.
+/// \param LHSTy The type being assigned to. Checks will only be performed if
+///              the type is a `counted_by` or `counted_by_or_null ` pointer.
+/// \param RHSExpr The expression being assigned from.
+/// \param Action The type assignment being performed
+/// \param Loc The SourceLocation to use for error diagnostics
+/// \param ComputeAssignee If provided this function will be called before
+///        emitting a diagnostic. The function should return the name of
+///        entity being assigned to or an empty string if this cannot be
+///        determined.
+///
+/// \returns True iff no diagnostic where emitted, false otherwise.
+static bool CheckAssignmentToCountAttrPtrWithIncompletePointeeTy(
+    Sema &S, QualType LHSTy, Expr *RHSExpr, AssignmentAction Action,
+    SourceLocation Loc, llvm::function_ref<std::string()> ComputeAssignee) {
+  NamedDecl *IncompleteTyDecl = nullptr;
+  const CountAttributedType *CATy = nullptr;
+  QualType PointeeTy;
+  if (!HasCountedByAttrOnIncompletePointee(LHSTy, &IncompleteTyDecl, &CATy,
+                                           &PointeeTy))
+    return true;
+  assert(CATy && !CATy->isCountInBytes() && !PointeeTy.isNull());
+
+  // It's not expected that the diagnostic be emitted in these cases.
+  // It's not necessarily a problem but we should catch when this starts
+  // to happen.
+  assert(Action != AssignmentAction::Converting &&
+         Action != AssignmentAction::Sending &&
+         Action != AssignmentAction::Casting &&
+         Action != AssignmentAction::Passing_CFAudited);
+
+  // By having users provide a function we only pay the cost of allocation the
+  // string and computing when a diagnostic is emitted.
+  std::string Assignee = ComputeAssignee ? ComputeAssignee() : "";
+  {
+    auto D = S.Diag(Loc, diag::err_counted_by_on_incomplete_type_on_assign)
+             << (int)Action << Assignee << (Assignee.size() > 0)
+             << isa<ImplicitValueInitExpr>(RHSExpr) << LHSTy
+             << CATy->getAttributeName(/*WithMacroPrefix=*/true) << PointeeTy
+             << CATy->isOrNull();
+
+    if (RHSExpr->getSourceRange().isValid())
+      D << RHSExpr->getSourceRange();
+  }
+
+  EmitIncompleteCountedByPointeeNotes(S, CATy, IncompleteTyDecl);
+  return false; // check failed
+}
+
+bool Sema::BoundsSafetyCheckAssignmentToCountAttrPtr(
+    QualType LHSTy, Expr *RHSExpr, AssignmentAction Action, SourceLocation Loc,
+    llvm::function_ref<std::string()> ComputeAssignee) {
+  return CheckAssignmentToCountAttrPtrWithIncompletePointeeTy(
+      *this, LHSTy, RHSExpr, Action, Loc, ComputeAssignee);
+}
+
+bool Sema::BoundsSafetyCheckInitialization(const InitializedEntity &Entity,
+                                           const InitializationKind &Kind,
+                                           AssignmentAction Action,
+                                           QualType LHSType, Expr *RHSExpr) {
+  bool ChecksPassed = true;
+  auto SL = Kind.getLocation();
+
+  // Note: We don't call `BoundsSafetyCheckAssignmentToCountAttrPtr` here
+  // because we need conditionalize what is checked. In downstream
+  // Clang `counted_by` is supported on variable definitions and in that
+  // implementation an error diagnostic will be emitted on the variable
+  // definition if the pointee is an incomplete type. To avoid warning about the
+  // same problem twice (once when the variable is defined, once when Sema
+  // checks the initializer) we skip checking the initializer if it's a
+  // variable.
+  if (Action == AssignmentAction::Initializing &&
+      Entity.getKind() != InitializedEntity::EK_Variable) {
+
+    if (!CheckAssignmentToCountAttrPtrWithIncompletePointeeTy(
+            *this, LHSType, RHSExpr, Action, SL, [&Entity]() -> std::string {
+              if (const auto *VD =
+                      dyn_cast_or_null<ValueDecl>(Entity.getDecl())) {
+                return VD->getQualifiedNameAsString();
+              }
+              return "";
+            })) {
+      ChecksPassed = false;
+
+      // It's not necessarily bad if this assert fails but we should catch
+      // if this happens.
+      assert(Entity.getKind() == InitializedEntity::EK_Member);
+    }
+  }
+
+  return ChecksPassed;
+}
+
+bool Sema::BoundsSafetyCheckUseOfCountAttrPtr(Expr *E) {
+  QualType T = E->getType();
+  if (!T->isPointerType())
+    return true;
+
+  const CountAttributedType *CATy = nullptr;
+  QualType PointeeTy;
+  NamedDecl *IncompleteTyDecl = nullptr;
+  if (!HasCountedByAttrOnIncompletePointee(T, &IncompleteTyDecl, &CATy,
+                                           &PointeeTy))
+    return true;
+  assert(CATy && !CATy->isCountInBytes() && !PointeeTy.isNull());
+
+  // Generate a string for the diagnostic that describes the "use".
+  // The string is specialized for direct calls to produce a better
+  // diagnostic.
+  StringRef DirectCallFn;
+  std::string UseStr;
+  if (const auto *CE = dyn_cast<CallExpr>(E->IgnoreParens())) {
+    if (const auto *FD = CE->getDirectCallee()) {
+      DirectCallFn = FD->getName();
+    }
+  }
+  int SelectExprKind = DirectCallFn.size() > 0 ? 1 : 0;
+  if (SelectExprKind) {
+    UseStr = DirectCallFn;
+  } else {
+    llvm::raw_string_ostream SS(UseStr);
+    E->printPretty(SS, nullptr, PrintingPolicy(getPrintingPolicy()));
+  }
+  assert(UseStr.size() > 0);
+
+  Diag(E->getBeginLoc(), diag::err_counted_by_on_incomplete_type_on_use)
+      << SelectExprKind << UseStr << T << PointeeTy
----------------
Sirraide wrote:

```suggestion
  SmallString<64> UseStr;
  bool IsDirectCall = false;
  if (const auto *CE = dyn_cast<CallExpr>(E->IgnoreParens())) {
    if (const auto *FD = CE->getDirectCallee()) {
      UseStr = FD->getName();
      IsDirectCall = true;
    }
  }
  
  if (!IsDirectCall) {
    llvm::raw_svector_ostream SS(UseStr);
    E->printPretty(SS, nullptr, getPrintingPolicy());
  }

  Diag(E->getBeginLoc(), diag::err_counted_by_on_incomplete_type_on_use)
      << IsDirectCall << UseStr << T << PointeeTy
```
I think we can simplify this a bit (and maybe use a `SmallString<>` so we don’t have to heap-allocate in as many cases). This should be equivalent unless I made a mistake somewhere, so please double-check.

https://github.com/llvm/llvm-project/pull/106321