Skip to content

[analyzer] Handle [[assume(cond)]] as __builtin_assume(cond) #101063

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vortex73
Copy link
Contributor

@vortex73 vortex73 commented Jul 29, 2024

@vortex73
Copy link
Contributor Author

@steakhal Kinda stuck... It builds properly but doesn't detect the assume attribute.

Copy link

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 13996378d81c8fa9a364aeaafd7382abbc1db83a 97de745341dfc17e7ad9c595d239346a9114a4c6 --extensions cpp -- clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
View the diff from clang-format here.
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index 1a4eb88fb6..8ab8b97d8e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -83,23 +83,23 @@ bool BuiltinFunctionChecker::evalCall(const CallEvent &Call,
   const Expr *CE = Call.getOriginExpr();
 
   if (const auto *AttrStmt = dyn_cast<AttributedStmt>(CE)) {
-      for (const Attr *I : AttrStmt->getAttrs()) {
-          if (const auto *AssumeAttr = dyn_cast<CXXAssumeAttr>(I)) {
-              const Expr *AssumeExpr = AssumeAttr->getAssumption();
-              SVal Arg = C.getSVal(AssumeExpr);
-              if (Arg.isUndef())
-                  return true;
-
-              state = state->assume(Arg.castAs<DefinedOrUnknownSVal>(), true);
-              if (!state) {
-                  C.generateSink(C.getState(), C.getPredecessor());
-                  return true;
-              }
-
-              C.addTransition(state);
-              return true;
-          } 
+    for (const Attr *I : AttrStmt->getAttrs()) {
+      if (const auto *AssumeAttr = dyn_cast<CXXAssumeAttr>(I)) {
+        const Expr *AssumeExpr = AssumeAttr->getAssumption();
+        SVal Arg = C.getSVal(AssumeExpr);
+        if (Arg.isUndef())
+          return true;
+
+        state = state->assume(Arg.castAs<DefinedOrUnknownSVal>(), true);
+        if (!state) {
+          C.generateSink(C.getState(), C.getPredecessor());
+          return true;
+        }
+
+        C.addTransition(state);
+        return true;
       }
+    }
   }
 
   if (isBuiltinLikeFunction(Call)) {

@steakhal
Copy link
Contributor

No, its cool. You are on the right track. Ill come back to you tomorrow.

@steakhal steakhal changed the title [Clang] add support for handling assume attributes [analyzer] Handle [[assume(cond)]] as __builtin_assume(cond) Jul 30, 2024
@steakhal
Copy link
Contributor

Alright, It's not as easy as I first thought.
There are multiple obstacles: We can't really use PostStmt<AttributedStmt> as the ExprEngine::Visit does not visit them. Now, if we would add support for them, the next problem would be that the CFG does not spell them, thus the ExprEngine still won't visit them :D

We could also patch the CFG, as it already has the CFGBuilder::VisitAttributedStmt function, but it doesn't seem to be that straight forward, as the comment suggests there:

// AttributedStmts for [[likely]] can have arbitrary statements as children,
// and the current visitation order here would add the AttributedStmts
// for [[likely]] after the child nodes, which is undesirable: For example,
// if the child contains an unconditional return, the [[likely]] would be
// considered unreachable.
// So only add the AttributedStmt for FallThrough, which has CFG effects and
// also no children, and omit the others. None of the other current StmtAttrs
// have semantic meaning for the CFG.

And for us, we would need to evaluate the operands of the AttributedStmt before we would evaluate the AttributedStmt itself - because the assumption probably refers to values of expression of child AST nodes.

So, it doesn't look easy :D My bad.
We could likely still make it work, but now I'm busy with something else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants