Skip to content

[BoundsSafety] Delay processing of bounds attrs in templates #9929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 13, 2025
Merged

[BoundsSafety] Delay processing of bounds attrs in templates #9929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c1b09a5
[BoundsSafety] Delay processing of bounds attrs in templates
hnrklssn Jan 31, 2025
46b00b0
address PR comments
hnrklssn Feb 1, 2025
9c63b4d
address PR comments
hnrklssn Feb 7, 2025
79b1c8c
test various nested templated contexts and fix bugs
hnrklssn Feb 11, 2025
8510657
[BoundsSafety] Peek through BoundsCheckExpr in single->bidi->counted
hnrklssn Feb 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/Sema/BoundsSafetySuggestions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1594,6 +1594,10 @@ void UnsafeOperationVisitor::
Current = PE->getSubExpr();
continue;
}
if (const auto *BCE = dyn_cast<BoundsCheckExpr>(Current)) {
Current = BCE->getGuardedExpr();
continue;
}
if (const auto *CE = dyn_cast<CastExpr>(Current)) {
if (CE->getCastKind() == clang::CK_BoundsSafetyPointerCast) {
// Found a cast we might want to warn about.
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/Sema/DynamicCountPointerAssignmentAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3484,6 +3484,9 @@ namespace clang {

void DynamicCountPointerAssignmentAnalysis::run() {
AnalysisDeclContext AC(/* AnalysisDeclContextManager */ nullptr, dcl);
if (dcl->isTemplated()) // delay processing until template has been
// instantiated
return;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it work? Will DynamicCountPointerAssignmentAnalysis::run be called again when dcl is instantiated?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a new function decl will be created for the instantiated version and all the normal analyses will be run over it

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this sufficient? We could have dependent expressions when the declaration itself is not templated:

  • Non-templated method of a templated struct referencing the template argument
  • Lambda inside a templated function referencing the template argument
  • Method of a non-templated struct nested inside a templated struct referencing the outer struct's template argument

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to an isTemplated call instead

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dumb question: what's the difference between using isTemplated and isDependentContext() to rule out templates?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isTemplated covers some extra cases:

bool Decl::isTemplated() const {
  // A declaration is templated if it is a template or a template pattern, or
  // is within (lexcially for a friend or local function declaration,
  // semantically otherwise) a dependent context.
  if (auto *AsDC = dyn_cast<DeclContext>(this))
    return AsDC->isDependentContext();
  auto *DC = getFriendObjectKind() || isLocalExternDecl()
      ? getLexicalDeclContext() : getDeclContext();
  return DC->isDependentContext() || isTemplateDecl() ||
         getDescribedTemplateParams();
}


CFG *cfg = AC.getCFG();
if (!cfg)
Expand Down
10 changes: 5 additions & 5 deletions clang/lib/Sema/SemaDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17302,6 +17302,11 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
// the declaration context below. Otherwise, we're unable to transform
// 'this' expressions when transforming immediate context functions.

/*TO_UPSTREAM(BoundsSafety) ON*/
if (LangOpts.BoundsSafety)
DynamicCountPointerAssignmentAnalysis(*this, dcl).run();
/*TO_UPSTREAM(BoundsSafety) OFF*/

if (!IsInstantiation)
PopDeclContext();

Expand All @@ -17325,11 +17330,6 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
if (FD && !FD->isDeleted())
checkTypeSupport(FD->getType(), FD->getLocation(), FD);

/*TO_UPSTREAM(BoundsSafety) ON*/
if (LangOpts.BoundsSafety)
DynamicCountPointerAssignmentAnalysis(*this, dcl).run();
/*TO_UPSTREAM(BoundsSafety) OFF*/

return dcl;
}

Expand Down
Loading