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

hnrklssn · 2025-01-31T06:45:49Z

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.

hnrklssn · 2025-01-31T06:47:06Z

@swift-ci please test llvm
@swift-ci please smoke test

patrykstefanski · 2025-01-31T21:29:02Z

clang/lib/Sema/DynamicCountPointerAssignmentAnalysis.cpp

@@ -3485,6 +3485,9 @@ namespace clang {

 void DynamicCountPointerAssignmentAnalysis::run() {
  AnalysisDeclContext AC(/* AnalysisDeclContextManager */ nullptr, dcl);
+  if (isa<TemplateDecl>(dcl)) // delay processing until template has been
+                              // instantiated
+    return;


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

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

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

Changed to an isTemplated call instead

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

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(); }

clang/lib/Sema/SemaDeclAttr.cpp

clang/lib/Sema/SemaExpr.cpp

clang/lib/Sema/SemaDeclAttr.cpp

patrykstefanski · 2025-01-31T23:08:41Z

CC @ziqingluo-90 since we were discussing templates recently.

hnrklssn · 2025-02-01T00:24:50Z

@swift-ci please test

hnrklssn · 2025-02-01T00:25:16Z

@swift-ci please test llvm

clang/lib/Sema/SemaDeclAttr.cpp

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

Xazax-hun

I am not sure non-templated entities nested in templated entities are correctly handled, the very least we should have some tests for that. Added some examples of such cases inline.

Xazax-hun · 2025-02-05T11:45:56Z

clang/lib/Sema/DynamicCountPointerAssignmentAnalysis.cpp

@@ -3485,6 +3485,9 @@ namespace clang {

 void DynamicCountPointerAssignmentAnalysis::run() {
  AnalysisDeclContext AC(/* AnalysisDeclContextManager */ nullptr, dcl);
+  if (isa<TemplateDecl>(dcl)) // delay processing until template has been
+                              // instantiated
+    return;


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

Xazax-hun · 2025-02-05T11:48:52Z

clang/lib/Sema/SemaDeclAttr.cpp

 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()) {


Same question here, wondering if this is sufficient for non-templated entities nested in templates.

Also replaced with a call to isTemplated

hnrklssn · 2025-02-11T04:56:50Z

@swift-ci please test llvm

hnrklssn · 2025-02-11T04:56:57Z

@swift-ci please smoke test

ziqingluo-90 · 2025-02-12T21:04:41Z

Just curious: will you allow T being replaced with an annotated type? E.g.,

template<typename T> void f(T x);

void g(int * __counted_by(10) p) {
  f(p);
}

hnrklssn · 2025-02-12T21:34:51Z

template void f(T x);

void g(int * __counted_by(10) p) {
f(p);
}

This is currently ignored. I think it's related to how these attributes are completely ignored in casts, even C-style casts. I don't have any plans to fix this right now, but I do think that it would be good to get there eventually.

hnrklssn · 2025-02-12T21:35:04Z

@swift-ci please test llvm

hnrklssn · 2025-02-12T21:35:12Z

@swift-ci please smoke test

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.

This analysis is now done after inserting BoundsCheckExpr, so we need to peek through them to emit these warnings where we previously only had a BoundsSafetyPointerCast.

hnrklssn · 2025-02-13T06:16:15Z

@swift-ci please test

hnrklssn · 2025-02-13T17:33:38Z

windows failure seems unrelated

hnrklssn · 2025-02-13T17:35:13Z

@swift-ci please test llvm

This is a cherry-pick of change that was landed to `stable/20240723` (swiftlang#9929) but not landed on the `next` branch. Aside from resolving merge conflicts it was also necessary to modify the `clang/test/BoundsSafety/(AST|Sema)/value-dependence.cpp` tests because they fail to work when the new bounds checks are enabled (rdar://150044760). To workaround that the test has been made to disable the new `return_size` bounds check when `-fbounds-safety` is enabled with C++. --- * [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. Conflicts: clang/include/clang/Sema/Sema.h clang/lib/Sema/SemaDecl.cpp clang/lib/Sema/SemaTemplateInstantiateDecl.cpp clang/test/SemaCXX/warn-unsafe-buffer-usage-count-attributed-pointer-argument.cpp rdar://150694971 (cherry picked from commit c87bc75)

This is a cherry-pick of change that was landed to `stable/20240723` (#9929) but not landed on the `next` branch. Aside from resolving merge conflicts it was also necessary to modify the `clang/test/BoundsSafety/(AST|Sema)/value-dependence.cpp` tests because they fail to work when the new bounds checks are enabled (rdar://150044760). To workaround that the test has been made to disable the new `return_size` bounds check when `-fbounds-safety` is enabled with C++. --- * [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. Conflicts: clang/include/clang/Sema/Sema.h clang/lib/Sema/SemaDecl.cpp clang/lib/Sema/SemaTemplateInstantiateDecl.cpp clang/test/SemaCXX/warn-unsafe-buffer-usage-count-attributed-pointer-argument.cpp rdar://150694971 (cherry picked from commit c87bc75)

hnrklssn requested review from delcypher, rapidsna and patrykstefanski January 31, 2025 06:45

hnrklssn requested a review from a team as a code owner January 31, 2025 06:45

hnrklssn added the clang:bounds-safety Issue relating to the experimental -fbounds-safety feature in Clang label Jan 31, 2025

patrykstefanski reviewed Jan 31, 2025

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaExpr.cpp Show resolved Hide resolved

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

patrykstefanski approved these changes Feb 3, 2025

View reviewed changes

Xazax-hun reviewed Feb 5, 2025

View reviewed changes

hnrklssn force-pushed the bounds-safety-value-dependence branch from 427922f to 9858ef7 Compare February 11, 2025 04:55

hnrklssn requested a review from a team as a code owner February 11, 2025 04:55

hnrklssn added 5 commits February 12, 2025 22:14

address PR comments

46b00b0

address PR comments

9c63b4d

test various nested templated contexts and fix bugs

79b1c8c

[BoundsSafety] Peek through BoundsCheckExpr in single->bidi->counted

8510657

This analysis is now done after inserting BoundsCheckExpr, so we need to peek through them to emit these warnings where we previously only had a BoundsSafetyPointerCast.

hnrklssn force-pushed the bounds-safety-value-dependence branch from 6cabde8 to 8510657 Compare February 13, 2025 06:15

hnrklssn merged commit c87bc75 into swiftlang:stable/20240723 Feb 13, 2025
4 of 5 checks passed

delcypher mentioned this pull request May 22, 2025

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

Merged

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

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

Uh oh!

Conversation

hnrklssn commented Jan 31, 2025

Uh oh!

hnrklssn commented Jan 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrykstefanski commented Jan 31, 2025

Uh oh!

hnrklssn commented Feb 1, 2025

Uh oh!

hnrklssn commented Feb 1, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Xazax-hun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hnrklssn commented Feb 11, 2025

Uh oh!

hnrklssn commented Feb 11, 2025

Uh oh!

ziqingluo-90 commented Feb 12, 2025

Uh oh!

hnrklssn commented Feb 12, 2025

Uh oh!

hnrklssn commented Feb 12, 2025

Uh oh!

hnrklssn commented Feb 12, 2025

Uh oh!

hnrklssn commented Feb 13, 2025

Uh oh!

hnrklssn commented Feb 13, 2025

Uh oh!

hnrklssn commented Feb 13, 2025

Uh oh!

Uh oh!

Uh oh!