[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-05 Thread Balazs Benics via cfe-commits

https://github.com/steakhal updated 
https://github.com/llvm/llvm-project/pull/80456

>From 3a11db7ce1e91daacb86e183e7137db7a6101c9b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Joly?= 
Date: Tue, 9 Aug 2022 23:21:18 +0200
Subject: [PATCH] [analyzer] Model Microsoft "__assume" in the same way as
 clang "__builtin_assume"

---
 .../Checkers/BuiltinFunctionChecker.cpp|  3 ++-
 clang/test/Analysis/builtin-functions.cpp  | 18 ++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index 61521c259ca90..01e46fa8591c0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -44,7 +44,8 @@ bool BuiltinFunctionChecker::evalCall(const CallEvent ,
   default:
 return false;
 
-  case Builtin::BI__builtin_assume: {
+  case Builtin::BI__builtin_assume:
+  case Builtin::BI__assume: {
 assert (Call.getNumArgs() > 0);
 SVal Arg = Call.getArgSVal(0);
 if (Arg.isUndef())
diff --git a/clang/test/Analysis/builtin-functions.cpp 
b/clang/test/Analysis/builtin-functions.cpp
index 37e522049b174..8719193e405c4 100644
--- a/clang/test/Analysis/builtin-functions.cpp
+++ b/clang/test/Analysis/builtin-functions.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 
-analyzer-checker=core,debug.ExprInspection %s -std=c++11 -verify
+// RUN: %clang_analyze_cc1 -triple x86_64-pc-windows-msvc19.11.0 
-fms-extensions -analyzer-checker=core,debug.ExprInspection %s -std=c++11 
-verify
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -65,6 +66,23 @@ void g(int i) {
   }
 }
 
+#ifdef _WIN32
+namespace ms {
+void f(int i) {
+  __assume(i < 10);
+  clang_analyzer_eval(i < 15); // expected-warning {{TRUE}}
+}
+
+void g(int i) {
+  if (i > 5) {
+__assume(i < 5);
+clang_analyzer_warnIfReached(); // Assumtion contradicts constraints.
+// We give up the analysis on this path.
+  }
+}
+} // namespace ms
+#endif
+
 void test_constant_p(void *ptr) {
   int i = 1;
   const int j = 2;

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-04 Thread via cfe-commits

https://github.com/NagyDonat approved this pull request.

LGTM, thanks for the update.

https://github.com/llvm/llvm-project/pull/80456
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-03 Thread Balazs Benics via cfe-commits

steakhal wrote:

> The code LGTM with some minor remarks. (Disclaimer: I'm not familiar with 
> these MS functions.)
> 
> I'm not sure whether these "builtin by Microsoft" functions are in scope for 
> "our" BuiltinFunctionChecker which previously only checked functions that are 
> recognized as `Builtin::BI__something` by Clang. (However, I can believe that 
> there is no better place for them and I don't think that they would cause 
> problems here.)

For me, this file would be where I would look to see how `__assume` is eval 
called. This is actually an unknown function in regular mode, but with 
`-fms-extensions` it's actually defined. Check the AST for this 
[example](https://godbolt.org/z/1xdbP8scx). So, techniquely, it's not a 
builtin, but feels like it.

About the remarks, yes, let's do [Clean As You 
Code](https://docs.sonarsource.com/sonarqube/latest/user-guide/clean-as-you-code/#what-is-clean-as-you-code)!
 I'm all in.


> Hey!
> 
> Thanks for looking into this!
> 
> Did you actually encounter this call in the wild? The reason I ask, because 
> their definition looks like this in the current version of `sal.h`:
> 
> ```
> #ifndef __analysis_assume // [
> #ifdef _PREFAST_ // [
> #define __analysis_assume(expr) __assume(expr)
> #else // ][
> #define __analysis_assume(expr)
> #endif // ]
> #endif // ]
> 
> #ifndef _Analysis_assume_ // [
> #ifdef _PREFAST_ // [
> #define _Analysis_assume_(expr) __assume(expr)
> #else // ][
> #define _Analysis_assume_(expr)
> #endif // ]
> #endif // ]
> ```
> 
> The basic idea is, when MSVC's analyzer is invoked, `_PREFAST_` is defined, 
> and these macros should expand to `__assume(expr)`. This makes me a bit 
> surprised if the original names are present in the preprocessed code.
> 
> There is no difference between `__analysis_assume` and `_Analysis_assume_`. 
> The former is following the naming conventions in SAL 1, the latter is 
> following the conventions of SAL 2. The latter is preferred in user code, but 
> MSVC still supports both spellings.

I think I've seen FPs on ChakraCore and on the DotnetRuntime and on other 
Windows-related projects defining some of their assert macros by wrapping 
`__analysis_assume`. I can't exactly recall ATM where exactly I've seen that, 
but I'll do a measurment to confirm that this PR actually improves the 
situation.

Anyways, it turns out the FP I wanted to fix actually expands into using 
`__builtin_trap()`, which is still not modeled in CSA :face_with_spiral_eyes: 
I'll create a separate PR for handling that.

I think this PR stands on it's own, and improves the situation - (I'll check 
and come back of course).
I hope `__assume()` takes a single parameter (of any type) and returns `void` 
though.

Sorry for not doing my homework in the beginning, and thanks for catching that!
I though it's gonna be an easy one and I fire off a PR on my tea-break.

https://github.com/llvm/llvm-project/pull/80456
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-03 Thread Balazs Benics via cfe-commits

https://github.com/steakhal updated 
https://github.com/llvm/llvm-project/pull/80456

>From 9065aec18b5b9c4d922b0650e709e71ed31b5a45 Mon Sep 17 00:00:00 2001
From: Balazs Benics 
Date: Fri, 2 Feb 2024 16:24:21 +0100
Subject: [PATCH 1/2] [analyzer] Teach analzer about ms __analyzer_assume(bool)
 and friends

See the MS docs:
https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/using-the--analysis-assume-function-to-suppress-false-defects
https://learn.microsoft.com/en-us/cpp/code-quality/how-to-specify-additional-code-information-by-using-analysis-assume

TBH, I don't really know what is the difference between the two APIs.
---
 .../Checkers/BuiltinFunctionChecker.cpp   | 57 +--
 clang/test/Analysis/builtin-functions.cpp | 22 +++
 2 files changed, 62 insertions(+), 17 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index 61521c259ca90..ea874c1529b3b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -14,6 +14,7 @@
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
@@ -26,10 +27,41 @@ namespace {
 class BuiltinFunctionChecker : public Checker {
 public:
   bool evalCall(const CallEvent , CheckerContext ) const;
+
+private:
+  const CallDescriptionSet MicrosoftAnalysisAssume{
+  {{"__analysis_assume"}, 1},
+  {{"_Analysis_assume_"}, 1},
+  };
+
+  void evalCallAssume(const CallEvent , CheckerContext ) const;
 };
 
 }
 
+void BuiltinFunctionChecker::evalCallAssume(const CallEvent ,
+CheckerContext ) const {
+  assert(Call.getNumArgs() > 0);
+  assert(Call.getResultType()->isVoidType());
+  SVal Arg = Call.getArgSVal(0);
+
+  if (Arg.isUndef())
+return; // Return true to model purity.
+
+  ProgramStateRef State = C.getState();
+  State = State->assume(Arg.castAs(), true);
+
+  // FIXME: do we want to warn here? Not right now. The most reports might
+  // come from infeasible paths, thus being false positives.
+  if (!State) {
+C.generateSink(C.getState(), C.getPredecessor());
+return;
+  }
+
+  C.addTransition(State);
+  return;
+}
+
 bool BuiltinFunctionChecker::evalCall(const CallEvent ,
   CheckerContext ) const {
   ProgramStateRef state = C.getState();
@@ -39,29 +71,20 @@ bool BuiltinFunctionChecker::evalCall(const CallEvent ,
 
   const LocationContext *LCtx = C.getLocationContext();
   const Expr *CE = Call.getOriginExpr();
+  bool ReturnsVoid = Call.getResultType()->isVoidType();
+
+  if (MicrosoftAnalysisAssume.contains(Call) && ReturnsVoid) {
+evalCallAssume(Call, C);
+return true;
+  }
 
   switch (FD->getBuiltinID()) {
   default:
 return false;
 
-  case Builtin::BI__builtin_assume: {
-assert (Call.getNumArgs() > 0);
-SVal Arg = Call.getArgSVal(0);
-if (Arg.isUndef())
-  return true; // Return true to model purity.
-
-state = state->assume(Arg.castAs(), true);
-// FIXME: do we want to warn here? Not right now. The most reports might
-// come from infeasible paths, thus being false positives.
-if (!state) {
-  C.generateSink(C.getState(), C.getPredecessor());
-  return true;
-}
-
-C.addTransition(state);
+  case Builtin::BI__builtin_assume:
+evalCallAssume(Call, C);
 return true;
-  }
-
   case Builtin::BI__builtin_unpredictable:
   case Builtin::BI__builtin_expect:
   case Builtin::BI__builtin_expect_with_probability:
diff --git a/clang/test/Analysis/builtin-functions.cpp 
b/clang/test/Analysis/builtin-functions.cpp
index 37e522049b174..4a26f82ffcb16 100644
--- a/clang/test/Analysis/builtin-functions.cpp
+++ b/clang/test/Analysis/builtin-functions.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 
-analyzer-checker=core,debug.ExprInspection %s -std=c++11 -verify
 
+void __analysis_assume(bool);
+void _Analysis_assume_(bool);
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
 
@@ -82,3 +84,23 @@ void test_constant_p(void *ptr) {
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 1); // expected-warning 
{{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(ptr == 0)); // expected-warning 
{{FALSE}}
 }
+
+void test_ms_analysis_assume(int *p) {
+  __analysis_assume(p);
+  if (!p) {
+clang_analyzer_warnIfReached(); // no-warning: dead code.
+  }
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  __analysis_assume(false);
+  

[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-02 Thread Gábor Horváth via cfe-commits

Xazax-hun wrote:

Hey!

Thanks for looking into this!

Did you actually encounter this call in the wild? The reason I ask, because 
their definition looks like this in the current version of `sal.h`:

```
#ifndef __analysis_assume // [
#ifdef _PREFAST_ // [
#define __analysis_assume(expr) __assume(expr)
#else // ][
#define __analysis_assume(expr)
#endif // ]
#endif // ]

#ifndef _Analysis_assume_ // [
#ifdef _PREFAST_ // [
#define _Analysis_assume_(expr) __assume(expr)
#else // ][
#define _Analysis_assume_(expr)
#endif // ]
#endif // ]
```

The basic idea is, when MSVC's analyzer is invoked, `_PREFAST_` is defined, and 
these macros should expand to `__assume(expr)`. This makes me a bit surprised 
if the original names are present in the preprocessed code.

There is no difference between `__analysis_assume` and `_Analysis_assume_`. The 
former is following the naming conventions in SAL 1, the latter is following 
the conventions of SAL 2. The latter is preferred in user code, but MSVC's 
toolchain still supports both spellings. 

https://github.com/llvm/llvm-project/pull/80456
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-02 Thread via cfe-commits

https://github.com/NagyDonat edited 
https://github.com/llvm/llvm-project/pull/80456
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-02 Thread via cfe-commits


@@ -26,10 +27,41 @@ namespace {
 class BuiltinFunctionChecker : public Checker {
 public:
   bool evalCall(const CallEvent , CheckerContext ) const;
+
+private:
+  const CallDescriptionSet MicrosoftAnalysisAssume{
+  {{"__analysis_assume"}, 1},
+  {{"_Analysis_assume_"}, 1},
+  };
+
+  void evalCallAssume(const CallEvent , CheckerContext ) const;
 };
 
 }
 
+void BuiltinFunctionChecker::evalCallAssume(const CallEvent ,
+CheckerContext ) const {
+  assert(Call.getNumArgs() > 0);
+  assert(Call.getResultType()->isVoidType());
+  SVal Arg = Call.getArgSVal(0);
+
+  if (Arg.isUndef())
+return; // Return true to model purity.
+
+  ProgramStateRef State = C.getState();
+  State = State->assume(Arg.castAs(), true);
+
+  // FIXME: do we want to warn here? Not right now. The most reports might
+  // come from infeasible paths, thus being false positives.

NagyDonat wrote:

I'm pretty sure that we don't want to warn here. This analyzer-specific 
hint/override is presumably used in code that's confusing for the analyzer, so 
let's trust it.

https://github.com/llvm/llvm-project/pull/80456
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-02 Thread via cfe-commits


@@ -26,10 +27,41 @@ namespace {
 class BuiltinFunctionChecker : public Checker {
 public:
   bool evalCall(const CallEvent , CheckerContext ) const;
+
+private:
+  const CallDescriptionSet MicrosoftAnalysisAssume{
+  {{"__analysis_assume"}, 1},
+  {{"_Analysis_assume_"}, 1},
+  };
+
+  void evalCallAssume(const CallEvent , CheckerContext ) const;
 };
 
 }
 
+void BuiltinFunctionChecker::evalCallAssume(const CallEvent ,
+CheckerContext ) const {
+  assert(Call.getNumArgs() > 0);
+  assert(Call.getResultType()->isVoidType());
+  SVal Arg = Call.getArgSVal(0);
+
+  if (Arg.isUndef())
+return; // Return true to model purity.
+
+  ProgramStateRef State = C.getState();
+  State = State->assume(Arg.castAs(), true);
+
+  // FIXME: do we want to warn here? Not right now. The most reports might
+  // come from infeasible paths, thus being false positives.
+  if (!State) {
+C.generateSink(C.getState(), C.getPredecessor());
+return;
+  }
+
+  C.addTransition(State);
+  return;

NagyDonat wrote:

```suggestion
```
This is the last line of a `void` function.

https://github.com/llvm/llvm-project/pull/80456
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-02 Thread via cfe-commits


@@ -26,10 +27,41 @@ namespace {
 class BuiltinFunctionChecker : public Checker {
 public:
   bool evalCall(const CallEvent , CheckerContext ) const;
+
+private:
+  const CallDescriptionSet MicrosoftAnalysisAssume{
+  {{"__analysis_assume"}, 1},
+  {{"_Analysis_assume_"}, 1},
+  };
+
+  void evalCallAssume(const CallEvent , CheckerContext ) const;
 };
 
 }
 
+void BuiltinFunctionChecker::evalCallAssume(const CallEvent ,
+CheckerContext ) const {
+  assert(Call.getNumArgs() > 0);
+  assert(Call.getResultType()->isVoidType());
+  SVal Arg = Call.getArgSVal(0);
+
+  if (Arg.isUndef())
+return; // Return true to model purity.

NagyDonat wrote:

`Return true` is inaccurate/confusing after a valueless `return;`

https://github.com/llvm/llvm-project/pull/80456
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-02 Thread via cfe-commits

https://github.com/NagyDonat edited 
https://github.com/llvm/llvm-project/pull/80456
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-02 Thread via cfe-commits

https://github.com/NagyDonat commented:

The code LGTM with some minor remarks, but I'm not familiar with these MS 
functions.

I'm not sure whether these "builtin by Microsoft" functions are in scope for 
"our" BuiltinFunctionChecker which previously only checked functions that are 
recognized as `Builtin::BI__something` by Clang. (However, I can believe that 
there is no better place for them and I don't think that they would cause 
problems here.)

https://github.com/llvm/llvm-project/pull/80456
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-02 Thread Balazs Benics via cfe-commits

steakhal wrote:

Let me know if this is correct @Xazax-hun. You probably have more insights on 
these APIs than me ;)

https://github.com/llvm/llvm-project/pull/80456
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-02 Thread via cfe-commits

llvmbot wrote:




@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balazs Benics (steakhal)


Changes

See the MS docs:
https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/using-the--analysis-assume-function-to-suppress-false-defects
 
https://learn.microsoft.com/en-us/cpp/code-quality/how-to-specify-additional-code-information-by-using-analysis-assume

TBH, I don't really know what is the difference between the two APIs.

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


2 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp 
(+40-17) 
- (modified) clang/test/Analysis/builtin-functions.cpp (+22) 


``diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index 61521c259ca90..ea874c1529b3b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -14,6 +14,7 @@
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
@@ -26,10 +27,41 @@ namespace {
 class BuiltinFunctionChecker : public Checker {
 public:
   bool evalCall(const CallEvent , CheckerContext ) const;
+
+private:
+  const CallDescriptionSet MicrosoftAnalysisAssume{
+  {{"__analysis_assume"}, 1},
+  {{"_Analysis_assume_"}, 1},
+  };
+
+  void evalCallAssume(const CallEvent , CheckerContext ) const;
 };
 
 }
 
+void BuiltinFunctionChecker::evalCallAssume(const CallEvent ,
+CheckerContext ) const {
+  assert(Call.getNumArgs() > 0);
+  assert(Call.getResultType()->isVoidType());
+  SVal Arg = Call.getArgSVal(0);
+
+  if (Arg.isUndef())
+return; // Return true to model purity.
+
+  ProgramStateRef State = C.getState();
+  State = State->assume(Arg.castAs(), true);
+
+  // FIXME: do we want to warn here? Not right now. The most reports might
+  // come from infeasible paths, thus being false positives.
+  if (!State) {
+C.generateSink(C.getState(), C.getPredecessor());
+return;
+  }
+
+  C.addTransition(State);
+  return;
+}
+
 bool BuiltinFunctionChecker::evalCall(const CallEvent ,
   CheckerContext ) const {
   ProgramStateRef state = C.getState();
@@ -39,29 +71,20 @@ bool BuiltinFunctionChecker::evalCall(const CallEvent ,
 
   const LocationContext *LCtx = C.getLocationContext();
   const Expr *CE = Call.getOriginExpr();
+  bool ReturnsVoid = Call.getResultType()->isVoidType();
+
+  if (MicrosoftAnalysisAssume.contains(Call) && ReturnsVoid) {
+evalCallAssume(Call, C);
+return true;
+  }
 
   switch (FD->getBuiltinID()) {
   default:
 return false;
 
-  case Builtin::BI__builtin_assume: {
-assert (Call.getNumArgs() > 0);
-SVal Arg = Call.getArgSVal(0);
-if (Arg.isUndef())
-  return true; // Return true to model purity.
-
-state = state->assume(Arg.castAs(), true);
-// FIXME: do we want to warn here? Not right now. The most reports might
-// come from infeasible paths, thus being false positives.
-if (!state) {
-  C.generateSink(C.getState(), C.getPredecessor());
-  return true;
-}
-
-C.addTransition(state);
+  case Builtin::BI__builtin_assume:
+evalCallAssume(Call, C);
 return true;
-  }
-
   case Builtin::BI__builtin_unpredictable:
   case Builtin::BI__builtin_expect:
   case Builtin::BI__builtin_expect_with_probability:
diff --git a/clang/test/Analysis/builtin-functions.cpp 
b/clang/test/Analysis/builtin-functions.cpp
index 37e522049b174..4a26f82ffcb16 100644
--- a/clang/test/Analysis/builtin-functions.cpp
+++ b/clang/test/Analysis/builtin-functions.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 
-analyzer-checker=core,debug.ExprInspection %s -std=c++11 -verify
 
+void __analysis_assume(bool);
+void _Analysis_assume_(bool);
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
 
@@ -82,3 +84,23 @@ void test_constant_p(void *ptr) {
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 1); // expected-warning 
{{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(ptr == 0)); // expected-warning 
{{FALSE}}
 }
+
+void test_ms_analysis_assume(int *p) {
+  __analysis_assume(p);
+  if (!p) {
+clang_analyzer_warnIfReached(); // no-warning: dead code.
+  }
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  __analysis_assume(false);
+  clang_analyzer_warnIfReached(); // no-warning: dead code.
+}
+
+void test_ms_Analysis_assume_(int *p) {
+  _Analysis_assume_(p);
+  if (!p) {
+

[clang] [analyzer] Teach analzer about ms __analyzer_assume(bool) and friends (PR #80456)

2024-02-02 Thread Balazs Benics via cfe-commits

https://github.com/steakhal created 
https://github.com/llvm/llvm-project/pull/80456

See the MS docs:
https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/using-the--analysis-assume-function-to-suppress-false-defects
 
https://learn.microsoft.com/en-us/cpp/code-quality/how-to-specify-additional-code-information-by-using-analysis-assume

TBH, I don't really know what is the difference between the two APIs.

>From 9065aec18b5b9c4d922b0650e709e71ed31b5a45 Mon Sep 17 00:00:00 2001
From: Balazs Benics 
Date: Fri, 2 Feb 2024 16:24:21 +0100
Subject: [PATCH] [analyzer] Teach analzer about ms __analyzer_assume(bool) and
 friends

See the MS docs:
https://learn.microsoft.com/en-us/windows-hardware/drivers/devtest/using-the--analysis-assume-function-to-suppress-false-defects
https://learn.microsoft.com/en-us/cpp/code-quality/how-to-specify-additional-code-information-by-using-analysis-assume

TBH, I don't really know what is the difference between the two APIs.
---
 .../Checkers/BuiltinFunctionChecker.cpp   | 57 +--
 clang/test/Analysis/builtin-functions.cpp | 22 +++
 2 files changed, 62 insertions(+), 17 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index 61521c259ca90..ea874c1529b3b 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -14,6 +14,7 @@
 #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
@@ -26,10 +27,41 @@ namespace {
 class BuiltinFunctionChecker : public Checker {
 public:
   bool evalCall(const CallEvent , CheckerContext ) const;
+
+private:
+  const CallDescriptionSet MicrosoftAnalysisAssume{
+  {{"__analysis_assume"}, 1},
+  {{"_Analysis_assume_"}, 1},
+  };
+
+  void evalCallAssume(const CallEvent , CheckerContext ) const;
 };
 
 }
 
+void BuiltinFunctionChecker::evalCallAssume(const CallEvent ,
+CheckerContext ) const {
+  assert(Call.getNumArgs() > 0);
+  assert(Call.getResultType()->isVoidType());
+  SVal Arg = Call.getArgSVal(0);
+
+  if (Arg.isUndef())
+return; // Return true to model purity.
+
+  ProgramStateRef State = C.getState();
+  State = State->assume(Arg.castAs(), true);
+
+  // FIXME: do we want to warn here? Not right now. The most reports might
+  // come from infeasible paths, thus being false positives.
+  if (!State) {
+C.generateSink(C.getState(), C.getPredecessor());
+return;
+  }
+
+  C.addTransition(State);
+  return;
+}
+
 bool BuiltinFunctionChecker::evalCall(const CallEvent ,
   CheckerContext ) const {
   ProgramStateRef state = C.getState();
@@ -39,29 +71,20 @@ bool BuiltinFunctionChecker::evalCall(const CallEvent ,
 
   const LocationContext *LCtx = C.getLocationContext();
   const Expr *CE = Call.getOriginExpr();
+  bool ReturnsVoid = Call.getResultType()->isVoidType();
+
+  if (MicrosoftAnalysisAssume.contains(Call) && ReturnsVoid) {
+evalCallAssume(Call, C);
+return true;
+  }
 
   switch (FD->getBuiltinID()) {
   default:
 return false;
 
-  case Builtin::BI__builtin_assume: {
-assert (Call.getNumArgs() > 0);
-SVal Arg = Call.getArgSVal(0);
-if (Arg.isUndef())
-  return true; // Return true to model purity.
-
-state = state->assume(Arg.castAs(), true);
-// FIXME: do we want to warn here? Not right now. The most reports might
-// come from infeasible paths, thus being false positives.
-if (!state) {
-  C.generateSink(C.getState(), C.getPredecessor());
-  return true;
-}
-
-C.addTransition(state);
+  case Builtin::BI__builtin_assume:
+evalCallAssume(Call, C);
 return true;
-  }
-
   case Builtin::BI__builtin_unpredictable:
   case Builtin::BI__builtin_expect:
   case Builtin::BI__builtin_expect_with_probability:
diff --git a/clang/test/Analysis/builtin-functions.cpp 
b/clang/test/Analysis/builtin-functions.cpp
index 37e522049b174..4a26f82ffcb16 100644
--- a/clang/test/Analysis/builtin-functions.cpp
+++ b/clang/test/Analysis/builtin-functions.cpp
@@ -1,5 +1,7 @@
 // RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 
-analyzer-checker=core,debug.ExprInspection %s -std=c++11 -verify
 
+void __analysis_assume(bool);
+void _Analysis_assume_(bool);
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
 
@@ -82,3 +84,23 @@ void test_constant_p(void *ptr) {
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 1); // expected-warning 
{{TRUE}}