[PATCH] D36251: [analyzer] Suppress warning when bug path contains noreturn function or return from main in MagentaHandleChecker

[Haowei Wu via Phabricator via cfe-commits]

haowei abandoned this revision.
haowei added a comment.
Herald added subscribers: steakhal, ASDenysPetrov, Charusso, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware.

An updated version was landed in f4c26d993bdc 
 . This 
diff is no longer needed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36251/new/

https://reviews.llvm.org/D36251

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

[PATCH] D36251: [analyzer] Suppress warning when bug path contains noreturn function or return from main in MagentaHandleChecker

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

I'm worried about doing this for main(). I think many people would find it 
surprising if analyzing main() were to behave differently than other functions. 
I'm particularly worried because I think it is quite common to create a small 
test program with only main() to understand the behavior of the analyzer.


https://reviews.llvm.org/D36251



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

[PATCH] D36251: [analyzer] Suppress warning when bug path contains noreturn function or return from main in MagentaHandleChecker

[Haowei Wu via Phabricator via cfe-commits]

haowei added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp:483-498
+void MagentaHandleChecker::checkPreStmt(const ReturnStmt *RS,
+CheckerContext &Ctx) const {
+  ProgramStateRef State = Ctx.getState();
+  const StackFrameContext *SFCtx = Ctx.getStackFrame();
+  if (!SFCtx || !SFCtx->inTopFrame())
+return;
+  const FunctionDecl *FD = dyn_cast_or_null(SFCtx->getDecl());

NoQ wrote:
> xazax.hun wrote:
> > NoQ wrote:
> > > I think the analyzer core should do this. This code already has a global 
> > > effect on the analysis - it's enough for one checker to generate the 
> > > sink. Additionally, there's also the CFG-based variant of 
> > > suppress-on-sink, which would need to be extended to support this as well 
> > > - this other variant kicks in when the analysis was interrupted before 
> > > reaching the sink (see D35673 and D35674).
> > Do we want to do this unconditionally? Are all of the resources cleaned up 
> > on all of the supported OSes, or maybe for some leak issues it still makes 
> > sense to warn in these cases? Or we simply favor false negatives over false 
> > positives in this case (might make sense)? 
> We could add a flag to `setSuppressOnSink()` as an orthogonal change if it 
> turns out that we need it; i'm not aware of any stuff that badly needs to be 
> cleaned up before normal program termination in the existing checkers.
I also think this should be done in a separate checker like 
NoReturnFunctionChecker or within the analyzer as some other resource leak 
checkers might need this. I am also thinking D36475 should be in a separate 
checker as well. That annotation support really helps us a lot in analyzing 
unit test code and it might be useful for others if someone run into similar 
situation like us. But I cannot decide if we should use the annotate attribute 
or define a new attribute just for the analyzer.


https://reviews.llvm.org/D36251



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

[PATCH] D36251: [analyzer] Suppress warning when bug path contains noreturn function or return from main in MagentaHandleChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp:483-498
+void MagentaHandleChecker::checkPreStmt(const ReturnStmt *RS,
+CheckerContext &Ctx) const {
+  ProgramStateRef State = Ctx.getState();
+  const StackFrameContext *SFCtx = Ctx.getStackFrame();
+  if (!SFCtx || !SFCtx->inTopFrame())
+return;
+  const FunctionDecl *FD = dyn_cast_or_null(SFCtx->getDecl());

xazax.hun wrote:
> NoQ wrote:
> > I think the analyzer core should do this. This code already has a global 
> > effect on the analysis - it's enough for one checker to generate the sink. 
> > Additionally, there's also the CFG-based variant of suppress-on-sink, which 
> > would need to be extended to support this as well - this other variant 
> > kicks in when the analysis was interrupted before reaching the sink (see 
> > D35673 and D35674).
> Do we want to do this unconditionally? Are all of the resources cleaned up on 
> all of the supported OSes, or maybe for some leak issues it still makes sense 
> to warn in these cases? Or we simply favor false negatives over false 
> positives in this case (might make sense)? 
We could add a flag to `setSuppressOnSink()` as an orthogonal change if it 
turns out that we need it; i'm not aware of any stuff that badly needs to be 
cleaned up before normal program termination in the existing checkers.


https://reviews.llvm.org/D36251



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

[PATCH] D36251: [analyzer] Suppress warning when bug path contains noreturn function or return from main in MagentaHandleChecker

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp:483-498
+void MagentaHandleChecker::checkPreStmt(const ReturnStmt *RS,
+CheckerContext &Ctx) const {
+  ProgramStateRef State = Ctx.getState();
+  const StackFrameContext *SFCtx = Ctx.getStackFrame();
+  if (!SFCtx || !SFCtx->inTopFrame())
+return;
+  const FunctionDecl *FD = dyn_cast_or_null(SFCtx->getDecl());

NoQ wrote:
> I think the analyzer core should do this. This code already has a global 
> effect on the analysis - it's enough for one checker to generate the sink. 
> Additionally, there's also the CFG-based variant of suppress-on-sink, which 
> would need to be extended to support this as well - this other variant kicks 
> in when the analysis was interrupted before reaching the sink (see D35673 and 
> D35674).
Do we want to do this unconditionally? Are all of the resources cleaned up on 
all of the supported OSes, or maybe for some leak issues it still makes sense 
to warn in these cases? Or we simply favor false negatives over false positives 
in this case (might make sense)? 


https://reviews.llvm.org/D36251



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

[PATCH] D36251: [analyzer] Suppress warning when bug path contains noreturn function or return from main in MagentaHandleChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp:483-498
+void MagentaHandleChecker::checkPreStmt(const ReturnStmt *RS,
+CheckerContext &Ctx) const {
+  ProgramStateRef State = Ctx.getState();
+  const StackFrameContext *SFCtx = Ctx.getStackFrame();
+  if (!SFCtx || !SFCtx->inTopFrame())
+return;
+  const FunctionDecl *FD = dyn_cast_or_null(SFCtx->getDecl());

I think the analyzer core should do this. This code already has a global effect 
on the analysis - it's enough for one checker to generate the sink. 
Additionally, there's also the CFG-based variant of suppress-on-sink, which 
would need to be extended to support this as well - this other variant kicks in 
when the analysis was interrupted before reaching the sink (see D35673 and 
D35674).


https://reviews.llvm.org/D36251



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

[PATCH] D36251: [analyzer] Suppress warning when bug path contains noreturn function or return from main in MagentaHandleChecker

[Haowei Wu via Phabricator via cfe-commits]

haowei created this revision.
Herald added a subscriber: xazax.hun.

This patch will suppress handle leak warnings if the path of a bug reported by 
MagentaHandleChecker will reach a no return function or will reach the end of 
main function. As these two scenarios mean program crash and the leaked handle 
will be recycled by OS, there is no point to report these bugs.


https://reviews.llvm.org/D36251

Files:
  lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp
  test/Analysis/mxhandle.c


Index: test/Analysis/mxhandle.c
===
--- test/Analysis/mxhandle.c
+++ test/Analysis/mxhandle.c
@@ -58,7 +58,20 @@
 void useHandle02(
 MX_SYSCALL_PARAM_ATTR(handle_use) mx_handle_t *handle);
 
+void noReturnFunc() __attribute__((__noreturn__));
+
+int leakingFunc(int argc, char* argv[]) {
+  mx_handle_t sa, sb;
+  mx_channel_create(0, &sa, &sb);
+  return 1;
+}
+
 // End of declaration
+int main(int argc, char* argv[]) {
+  if (argc == 2)
+return leakingFunc(argc, argv); // Should not report any bugs here
+  return 0;
+}
 
 void checkNoLeak01() {
   mx_handle_t sa, sb;
@@ -207,6 +220,14 @@
   mx_handle_close(sb);
 }
 
+void checkNoLeak16() {
+  mx_handle_t sa, sb;
+  if (mx_channel_create(0, &sa, &sb) < 0) {
+return;
+  }
+  noReturnFunc(); // Should not report any bugs here
+}
+
 void checkLeak01() {
   mx_handle_t sa, sb;
   mx_channel_create(0, &sa, &sb);
Index: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp
@@ -239,7 +239,7 @@
 
 class MagentaHandleChecker
 : public Checker {
+ check::PostCall, check::PreStmt> {
   std::unique_ptr LeakBugType;
   std::unique_ptr DoubleReleaseBugType;
   std::unique_ptr UseAfterFreeBugType;
@@ -255,6 +255,7 @@
   void checkPostCall(const CallEvent &Call, CheckerContext &Ctx) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &Ctx) const;
   bool evalCall(const CallExpr *CE, CheckerContext &Ctx) const;
+  void checkPreStmt(const ReturnStmt *RS, CheckerContext &Ctx) const;
   ProgramStateRef checkPointerEscape(ProgramStateRef State,
  const InvalidatedSymbols &Escaped,
  const CallEvent *Call,
@@ -402,6 +403,7 @@
   // Initialize the bug types.
   LeakBugType.reset(
   new BugType(this, "MX handle leak", "Magenta Handle Error"));
+  LeakBugType->setSuppressOnSink(true);
   DoubleReleaseBugType.reset(
   new BugType(this, "MX handle double release", "Magenta Handle Error"));
   UseAfterFreeBugType.reset(
@@ -478,6 +480,23 @@
   }
 }
 
+void MagentaHandleChecker::checkPreStmt(const ReturnStmt *RS,
+CheckerContext &Ctx) const {
+  ProgramStateRef State = Ctx.getState();
+  const StackFrameContext *SFCtx = Ctx.getStackFrame();
+  if (!SFCtx || !SFCtx->inTopFrame())
+return;
+  const FunctionDecl *FD = dyn_cast_or_null(SFCtx->getDecl());
+  if (!FD)
+return;
+  const IdentifierInfo *II = FD->getIdentifier();
+  if (!II)
+return;
+
+  if (II->isStr("main"))
+Ctx.generateSink(State, Ctx.getPredecessor());
+}
+
 ProgramStateRef MagentaHandleChecker::checkPointerEscape(
 ProgramStateRef State, const InvalidatedSymbols &Escaped,
 const CallEvent *Call, PointerEscapeKind Kind) const {


Index: test/Analysis/mxhandle.c
===
--- test/Analysis/mxhandle.c
+++ test/Analysis/mxhandle.c
@@ -58,7 +58,20 @@
 void useHandle02(
 MX_SYSCALL_PARAM_ATTR(handle_use) mx_handle_t *handle);
 
+void noReturnFunc() __attribute__((__noreturn__));
+
+int leakingFunc(int argc, char* argv[]) {
+  mx_handle_t sa, sb;
+  mx_channel_create(0, &sa, &sb);
+  return 1;
+}
+
 // End of declaration
+int main(int argc, char* argv[]) {
+  if (argc == 2)
+return leakingFunc(argc, argv); // Should not report any bugs here
+  return 0;
+}
 
 void checkNoLeak01() {
   mx_handle_t sa, sb;
@@ -207,6 +220,14 @@
   mx_handle_close(sb);
 }
 
+void checkNoLeak16() {
+  mx_handle_t sa, sb;
+  if (mx_channel_create(0, &sa, &sb) < 0) {
+return;
+  }
+  noReturnFunc(); // Should not report any bugs here
+}
+
 void checkLeak01() {
   mx_handle_t sa, sb;
   mx_channel_create(0, &sa, &sb);
Index: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp
@@ -239,7 +239,7 @@
 
 class MagentaHandleChecker
 : public Checker {
+ check::PostCall, check::PreStmt> {
   std::unique_ptr LeakBugType;
   std::unique_ptr DoubleReleaseBugType;
   std::unique_ptr UseAfterFreeBugType;
@@ -255,6 +255,7 @@
   void checkPostCall(const CallEvent &Call, Checke