Skip to content

[Clang] FunctionEffects: Correctly navigate through array types in FunctionEffectsRef::get(). #121525

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.

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
Jan 17, 2025

Conversation

dougsonos
Copy link
Contributor

@dougsonos dougsonos commented Jan 2, 2025

FunctionEffectsRef::get() is supposed to strip off layers of indirection (pointers/references, type sugar) to get to a FunctionProtoType (if any) and return its effects (if any).

It wasn't correctly dealing with situations where the compiler implicitly converts an array to a pointer.

@dougsonos dougsonos marked this pull request as ready for review January 3, 2025 00:59
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 3, 2025
@dougsonos dougsonos self-assigned this Jan 3, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 3, 2025

@llvm/pr-subscribers-clang

Author: Doug Wyatt (dougsonos)

Changes

FunctionEffectsRef::get() is supposed to strip off layers of indirection (pointers/references, type sugar) to get to a FunctionProtoType (if any) and return its effects (if any).

It wasn't correctly dealing with situations where the compiler implicitly converts an array to a pointer.


Full diff: https://github.com/llvm/llvm-project/pull/121525.diff

2 Files Affected:

  • (modified) clang/include/clang/AST/Type.h (+13-4)
  • (modified) clang/test/Sema/attr-nonblocking-constraints.cpp (+11)
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 09c98f642852fc..782c32f41852e2 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -8836,13 +8836,22 @@ void FixedPointValueToString(SmallVectorImpl<char> &Str, llvm::APSInt Val,
                              unsigned Scale);
 
 inline FunctionEffectsRef FunctionEffectsRef::get(QualType QT) {
+  const Type *TypePtr = QT.getTypePtr();
   while (true) {
-    QualType Pointee = QT->getPointeeType();
-    if (Pointee.isNull())
+    // Note that getPointeeType() seems to successfully navigate some constructs
+    // for which isAnyPointerType() returns false (e.g.
+    // pointer-to-member-function).
+    QualType Pointee = TypePtr->getPointeeType();
+    if (Pointee.isNull()) {
+      if (TypePtr->isArrayType()) {
+        TypePtr = TypePtr->getBaseElementTypeUnsafe();
+        continue;
+      }
       break;
-    QT = Pointee;
+    }
+    TypePtr = Pointee.getTypePtr();
   }
-  if (const auto *FPT = QT->getAs<FunctionProtoType>())
+  if (const auto *FPT = TypePtr->getAs<FunctionProtoType>())
     return FPT->getFunctionEffects();
   return {};
 }
diff --git a/clang/test/Sema/attr-nonblocking-constraints.cpp b/clang/test/Sema/attr-nonblocking-constraints.cpp
index bbc909f627f4c3..f7b5abbfa34e91 100644
--- a/clang/test/Sema/attr-nonblocking-constraints.cpp
+++ b/clang/test/Sema/attr-nonblocking-constraints.cpp
@@ -246,6 +246,17 @@ void PTMFTester::convert() [[clang::nonblocking]]
 	(this->*mConvertFunc)();
 }
 
+// Allow implicit conversion from array to pointer.
+void nb14(unsigned idx) [[clang::nonblocking]]
+{
+	using FP = void (*)() [[clang::nonblocking]];
+	using FPArray = FP[2];
+	auto nb = +[]() [[clang::nonblocking]] {};
+
+	FPArray src{ nb, nullptr };
+	FP f = src[idx]; // This should not generate a warning.
+}
+
 // Block variables
 void nb17(void (^blk)() [[clang::nonblocking]]) [[clang::nonblocking]] {
 	blk();

@dougsonos dougsonos requested a review from Sirraide January 3, 2025 01:01
@dougsonos
Copy link
Contributor Author

Ping

Copy link

github-actions bot commented Jan 16, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Add tests for multi-dimensional and variable-length arrays.
Copy link
Member

@Sirraide Sirraide left a comment

Choose a reason for hiding this comment

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

One nit but lgtm otherwise

@dougsonos dougsonos merged commit 0417cd1 into llvm:main Jan 17, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants