Effect analysis: correctly detect (x ? a : b) as nonblocking when a and b are

[dougsonos]

Fix: Effect analysis: correctly detect (x ? a : b) as nonblocking when a and b are

[llvmbot]

@llvm/pr-subscribers-clang

Author: Doug Wyatt (dougsonos)

Changes

Fix: Effect analysis: correctly detect (f ? a : b) as nonblocking when a and b are

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaFunctionEffects.cpp (+5-6)
• (modified) clang/test/Sema/attr-nonblocking-constraints.cpp (+10)

diff --git a/clang/lib/Sema/SemaFunctionEffects.cpp b/clang/lib/Sema/SemaFunctionEffects.cpp
index 0fb18d207a50ba..0ac5de29f66aa7 100644
--- a/clang/lib/Sema/SemaFunctionEffects.cpp
+++ b/clang/lib/Sema/SemaFunctionEffects.cpp
@@ -1048,15 +1048,14 @@ class Analyzer {
     }
 
     void checkIndirectCall(CallExpr *Call, QualType CalleeType) {
-      auto *FPT =
-          CalleeType->getAs<FunctionProtoType>(); // Null if FunctionType.
       FunctionEffectKindSet CalleeEffects;
-      if (FPT)
-        CalleeEffects.insert(FPT->getFunctionEffects());
+      if (FunctionEffectsRef Effects = FunctionEffectsRef::get(CalleeType);
+          !Effects.empty())
+        CalleeEffects.insert(Effects);
 
       auto Check1Effect = [&](FunctionEffect Effect, bool Inferring) {
-        if (FPT == nullptr || Effect.shouldDiagnoseFunctionCall(
-                                  /*direct=*/false, CalleeEffects))
+        if (Effect.shouldDiagnoseFunctionCall(
+                /*direct=*/false, CalleeEffects))
           addViolation(Inferring, Effect, ViolationID::CallsExprWithoutEffect,
                        Call->getBeginLoc());
       };
diff --git a/clang/test/Sema/attr-nonblocking-constraints.cpp b/clang/test/Sema/attr-nonblocking-constraints.cpp
index c694860069c960..ff8caf0e573403 100644
--- a/clang/test/Sema/attr-nonblocking-constraints.cpp
+++ b/clang/test/Sema/attr-nonblocking-constraints.cpp
@@ -156,6 +156,16 @@ void nb10(
 	static_cast<void (*)()>(fp1)(); // expected-warning {{function with 'nonblocking' attribute must not call non-'nonblocking' expression}}
 }
 
+// Expression involving indirection
+int nb10a() [[clang::nonblocking]];
+int nb10b() [[clang::nonblocking]];
+
+int nb10c(bool x) [[clang::nonblocking]]
+{
+	// Warns that the expression is not nonblocking.
+	return (x ? nb10a : nb10b)();
+}
+
 // Interactions with nonblocking(false)
 void nb11_no_inference_1() [[clang::nonblocking(false)]] // expected-note {{function does not permit inference of 'nonblocking'}}
 {

[dougsonos]

FunctionEffectsRef::get() knows how to pick through indirections (references/pointers) to get to a FunctionProtoType so just reuse that.

[dougsonos]

The check for a null FPT is/was superfluous; in that case the callee never has any declared effects and shouldDiagnoseFunctionCall will complain.

[dougsonos]

@cjappl @Sirraide would appreciate what I hope will be a quick review on a small fix.

[cjappl]

LGTM.

Will let a little time pass to let Sirraide chime in, because this section of the code isn't my forte. If you want this merged ASAP @dougsonos and you feel confident in it, ping me and I can merge.

[Sirraide]

LGTM

[Sirraide]

@dougsonos Maybe you should ask for commit access since you’ll likely be contributing more patches to this in the future as I understand it?

CC @AaronBallman Does that still work like it used to or did your maintainers rfc change something about that too?

[Sirraide]

Oh, also, are there any other places where we should maybe be using FunctionEffectsRef::get()?

[dougsonos]

Oh, also, are there any other places where we should maybe be using FunctionEffectsRef::get()?

Good question. I looked for other uses of QualType and getType() in SemaFunctionEffects, but this is the only spot (checkIndirectCall) where we are interested in obtaining effects from an expression which didn't come from a Decl. If there were a Decl it would have taken an earlier branch in VisitCallExpr, where we build a CallableInfo, whose constructor calls FunctionEffectsRef::get() in the case of an indirect call through a variable which has a Decl. So I think we're good.

Thanks for the quick review.

More patches? My favorite of Murphy's Laws of Programming is "if it is useful, it will have to be changed."

[cjappl]

@dougsonos Maybe you should ask for commit access since you’ll likely be contributing more patches to this in the future as I understand it?

CC @AaronBallman Does that still work like it used to or did your maintainers rfc change something about that too?

A few days ago at least, the other RTSan coauthor followed the "old way" of emailing Chris Lattner to get access. He followed the same process I did a few months ago (email him asking for access with a list of commits you have contributed).

Not sure if that has changed in the last week or not.