[PATCH] D142354: [analyzer] Create a stub for an std::variant checker

[Gábor Spaits via Phabricator via cfe-commits]

spaits added a comment.

Thank you for your response @steakhal! I am going to define a mock standard 
variant header.
I was thinking about just copying the definitions from the original variant 
header
but it had other headers as dependency. I will write my own variant definition 
one step at the time.
First I will start with a very simple one. Then I am going to add the 
definitions
that are needed to test the latest changes in the checker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142354

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

[PATCH] D142354: [analyzer] Create a stub for an std::variant checker

[Gábor Spaits via Phabricator via cfe-commits]

spaits updated this revision to Diff 491816.
spaits added a comment.

Adding mock header for std::variant.


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

https://reviews.llvm.org/D142354

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
  clang/test/Analysis/Inputs/variant.h
  clang/test/Analysis/std-variant-checker.cpp

Index: clang/test/Analysis/std-variant-checker.cpp
===
--- /dev/null
+++ clang/test/Analysis/std-variant-checker.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.StdVariant %s -verify
+
+#include "Inputs/variant.h"
+
+void g() {
+  std::variant v; // expected-warning{{Variant Created [alpha.core.StdVariant]}}
+}
\ No newline at end of file
Index: clang/test/Analysis/Inputs/variant.h
===
--- /dev/null
+++ clang/test/Analysis/Inputs/variant.h
@@ -0,0 +1,16 @@
+// Like the compiler, the static analyzer treats some functions differently if
+// they come from a system header -- for example, it is assumed that system
+// functions do not arbitrarily free() their parameters, and that some bugs
+// found in system headers cannot be fixed by the user and should be
+// suppressed.
+#pragma clang system_header
+
+#ifndef VARIANT_H
+#define VARIANT_H
+
+namespace std {
+  template
+  class variant {};
+} //end of namespace std
+
+#endif //VARIANT_H
\ No newline at end of file
Index: clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
@@ -0,0 +1,50 @@
+//===- StdVariantChecker.cpp -*- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.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"
+
+using namespace clang;
+using namespace ento;
+
+class StdVariantChecker : public Checker {
+CallDescription VariantConstructorCall{{"std", "variant"}, 0, 0};
+BugType VariantCreated{this, "VariantCreated", "VariantCreated"};
+
+public:
+void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+  if (!isa(Call))
+return;
+  if (!VariantConstructorCall.matches(Call))
+return;
+
+  ExplodedNode* ErrNode = C.generateNonFatalErrorNode();
+  if (!ErrNode)
+return;
+  llvm::SmallString<128> Str;
+  llvm::raw_svector_ostream OS(Str);
+  OS << "Variant Created";
+  auto R = std::make_unique(
+  VariantCreated, OS.str(), ErrNode);
+  C.emitReport(std::move(R));
+}
+};
+
+bool clang::ento::shouldRegisterStdVariantChecker(
+clang::ento::CheckerManager const &mgr) {
+  return true;
+}
+
+void clang::ento::registerStdVariantChecker(clang::ento::CheckerManager &mgr) {
+  mgr.registerChecker();
+}
\ No newline at end of file
Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
===
--- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -107,6 +107,7 @@
   SmartPtrModeling.cpp
   StackAddrEscapeChecker.cpp
   StdLibraryFunctionsChecker.cpp
+  StdVariantChecker.cpp
   STLAlgorithmModeling.cpp
   StreamChecker.cpp
   StringChecker.cpp
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -305,6 +305,10 @@
   Dependencies<[PthreadLockBase]>,
   Documentation;
 
+def StdVariantChecker : Checker<"StdVariant">,
+  HelpText<"Check std::variant">,
+  Documentation;
+
 } // end "alpha.core"
 
 //===--===//
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D142354: [analyzer] Create a stub for an std::variant checker

[Gábor Spaits via Phabricator via cfe-commits]

spaits created this revision.
spaits added reviewers: Szelethus, steakhal, NoQ, gamesh411, xazax.hun.
spaits added a project: clang.
Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.
Herald added a project: All.
spaits requested review of this revision.
Herald added a subscriber: cfe-commits.

As per the discussion on this thread i have started working on an std::variant 
checker.
https://discourse.llvm.org/t/analyzer-new-checker-for-std-any-as-a-bsc-thesis/65613/2

This patch is mostly a conversation starter. @Szelethus will supervise me on 
this project, and we shall discuss our next steps here.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142354

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
  clang/test/Analysis/std-variant-checker.cpp


Index: clang/test/Analysis/std-variant-checker.cpp
===
--- /dev/null
+++ clang/test/Analysis/std-variant-checker.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.StdVariant %s 
-verify
+
+namespace std {
+  template
+  class variant {};
+} //end of namespace std
+
+void g() {
+  std::variant v; // expected-warning{{Variant Created 
[alpha.core.StdVariant]}}
+}
\ No newline at end of file
Index: clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp
@@ -0,0 +1,51 @@
+//===- StdVariantChecker.cpp -*- C++ 
-*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.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"
+
+using namespace clang;
+using namespace ento;
+
+class StdVariantChecker : public Checker {
+CallDescription VariantConstructorCall{{"std", "variant"}, 0, 0};
+BugType VariantCreated{this, "VariantCreated", "VariantCreated"};
+
+public:
+void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+  if (!isa(Call))
+return;
+
+  if (!VariantConstructorCall.matches(Call))
+return;
+
+  ExplodedNode* ErrNode = C.generateNonFatalErrorNode();
+  if (!ErrNode)
+return;
+  llvm::SmallString<128> Str;
+  llvm::raw_svector_ostream OS(Str);
+  OS << "Variant Created";
+  auto R = std::make_unique(
+  VariantCreated, OS.str(), ErrNode);
+  C.emitReport(std::move(R));
+}
+};
+
+bool clang::ento::shouldRegisterStdVariantChecker(
+clang::ento::CheckerManager const &mgr) {
+  return true;
+}
+
+void clang::ento::registerStdVariantChecker(clang::ento::CheckerManager &mgr) {
+  mgr.registerChecker();
+}
\ No newline at end of file
Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
===
--- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -107,6 +107,7 @@
   SmartPtrModeling.cpp
   StackAddrEscapeChecker.cpp
   StdLibraryFunctionsChecker.cpp
+  StdVariantChecker.cpp
   STLAlgorithmModeling.cpp
   StreamChecker.cpp
   StringChecker.cpp
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -305,6 +305,10 @@
   Dependencies<[PthreadLockBase]>,
   Documentation;
 
+def StdVariantChecker : Checker<"StdVariant">,
+  HelpText<"Check std::variant">,
+  Documentation;
+
 } // end "alpha.core"
 
 
//===--===//


Index: clang/test/Analysis/std-variant-checker.cpp
===
--- /dev/null
+++ clang/test/Analysis/std-variant-checker.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.StdVariant %s -verify
+
+namespace std {
+  template
+  class variant {};
+} //end of namespace std
+
+void g() {
+  std::variant v; // expected-warning{{Variant Created [alpha.core.StdVariant]}}
+}
\ 

[PATCH] D145069: [analyzer][NFC] Split the no state change logic and bug report suppression into two visitors

[Gábor Spaits via Phabricator via cfe-commits]

spaits added a comment.

In D145069#4191046 , @xazax.hun wrote:

> Do you plan to selectively enable warnings coming from the STL to catch 
> misuses of certain STL types?

No. At first when we have found out that the Static Analyzer can reason about 
std::variant but it suppresses the diagnostics (D142354#4079643 
), we were suspecting a too strong 
heuristic somewhere, that invalidates even true positives. Since the Static 
Analyzer was able to reason about std::variant by itself we we did not want to 
write a checker to do the same thing. So the plan was to find the point where 
the suppression happen and change on the heuristics so it can let thru every 
kind of true positive about STL types/functions. But as it turns out, it is 
kind of impossible to do that without letting a lot of false positives to not 
be suppressed.

While it seems like it may be very difficult to unsuppress all the reports from 
std::variant, it still made sense to fine-tune some of these suppression.

In D145069#4191046 , @xazax.hun wrote:

> Also, those warning reports might be leaky in a sense the reported path might 
> contain implementations details from the STL that is hard to interpret.

We have tested the new heuristics and the new reports did not contain 
implementation details form STL.

In D145069#4191046 , @xazax.hun wrote:

> I am afraid, if we want to provide a good user experience, we might be doomed 
> to manually simulate the behavior of STL classes.

That might be the best approach to prevent the mentioned implementation 
dependency. Plus it would probably make it easier to write my BSc thesis if I 
have created a brand new checker.

On another note, it might be interesting to see how the checker approach and 
the force-inlining analyses compare (even if one of those approaches turn out 
to be a dead end).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145069

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

[PATCH] D145069: [analyzer][NFC] Split the no state change logic and bug report suppression into two visitors

[Gábor Spaits via Phabricator via cfe-commits]

spaits created this revision.
spaits added reviewers: NoQ, steakhal, Szelethus.
Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: All.
spaits requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

As discussed in my previous patch (D142354 ) 
the analyzer can understand std::variant and std::any through regular inlining. 
However warnings coming from these classes were suppressed by 
NoStateChangeFuncVisitor, more specificly by an instance of NoStoreFuncVisitor. 
The reason behind that is that we suppress bug reports when a system header 
function was supposed to initialize its parameter but failed to (as you can 
read about it in a comment block visible in this patch).

Now the problem is that these bug report were suppressed, but they had nothing 
to do with uninitialized values. In a follow up patch I intend to fix that and 
see how this suppression logic works as it was meant to be, but my initial 
testing shows that they no longer falsely suppress reports like this:

  std::variant v = 0;
  int i = std::get(v);
  10/i; // FIXME: This should warn for division by zero!

In this patch I only separated these two functionalities because I don't think 
this suppression should be implemented in a NoStateChangeVisitor but in it's 
own.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145069

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -452,6 +452,58 @@
   CallEventRef<> Call =
   BR.getStateManager().getCallEventManager().getCaller(SCtx, State);
 
+  if (Call->isInSystemHeader())
+return nullptr;
+
+  if (const auto *MC = dyn_cast(Call)) {
+// If we failed to construct a piece for self, we still want to check
+// whether the entity of interest is in a parameter.
+if (PathDiagnosticPieceRef Piece = maybeEmitNoteForObjCSelf(R, *MC, N))
+  return Piece;
+  }
+
+  if (const auto *CCall = dyn_cast(Call)) {
+// Do not generate diagnostics for not modified parameters in
+// constructors.
+return maybeEmitNoteForCXXThis(R, *CCall, N);
+  }
+
+  return maybeEmitNoteForParameters(R, *Call, N);
+}
+
+//===--===//
+// Implementation of SuppressSystemHeaderWarningVistor.
+//===--===//
+
+namespace {
+class SuppressSystemHeaderWarningVisitor : public BugReporterVisitor {
+public:
+  virtual PathDiagnosticPieceRef VisitNode(const ExplodedNode *,
+   BugReporterContext &,
+   PathSensitiveBugReport &) override;
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+static int Tag = 0;
+ID.AddPointer(&Tag);
+  }
+};
+} // namespace
+
+PathDiagnosticPieceRef
+SuppressSystemHeaderWarningVisitor::VisitNode(const ExplodedNode *Succ,
+  BugReporterContext &BRC,
+  PathSensitiveBugReport &BR) {
+  const LocationContext *Ctx = Succ->getLocationContext();
+  const StackFrameContext *SCtx = Ctx->getStackFrame();
+  ProgramStateRef State = Succ->getState();
+  auto CallExitLoc = Succ->getLocationAs();
+
+  // No diagnostic if region was modified inside the frame.
+  if (!CallExitLoc)
+return nullptr;
+
+  CallEventRef<> Call =
+  BRC.getStateManager().getCallEventManager().getCaller(SCtx, State);
+
   // Optimistically suppress uninitialized value bugs that result
   // from system headers having a chance to initialize the value
   // but failing to do so. It's too unlikely a system header's fault.
@@ -469,27 +521,13 @@
 // One common example of a standard function that doesn't ever initialize
 // its out parameter is operator placement new; it's up to the follow-up
 // constructor (if any) to initialize the memory.
-if (!N->getStackFrame()->getCFG()->isLinear()) {
+
+if (!Succ->getStackFrame()->getCFG()->isLinear()) {
   static int i = 0;
-  R.markInvalid(&i, nullptr);
+  BR.markInvalid(&i, nullptr);
 }
-return nullptr;
-  }
-
-  if (const auto *MC = dyn_cast(Call)) {
-// If we failed to construct a piece for self, we still want to check
-// whether the entity of interest is in a parameter.
-if (PathDiagnosticPieceRef Piece = maybeEmitNoteForObjCSelf(R, *MC, N))
-  return Piece;
-  }
-
-  if (const auto *CCall = dyn_cast(Call)) {
-// Do not generate diagnostics for not modified parameters in
-// constructors.
-return maybeEmitNote

[PATCH] D145069: [analyzer][NFC] Split the no state change logic and bug report suppression into two visitors

[Gábor Spaits via Phabricator via cfe-commits]

spaits updated this revision to Diff 504062.
spaits added a comment.

As @Szelethus has mentioned in his reply I tried to improve on the comment. I 
hope now it describes correctly what the new visitor is good for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145069

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -452,29 +452,8 @@
   CallEventRef<> Call =
   BR.getStateManager().getCallEventManager().getCaller(SCtx, State);
 
-  // Optimistically suppress uninitialized value bugs that result
-  // from system headers having a chance to initialize the value
-  // but failing to do so. It's too unlikely a system header's fault.
-  // It's much more likely a situation in which the function has a failure
-  // mode that the user decided not to check. If we want to hunt such
-  // omitted checks, we should provide an explicit function-specific note
-  // describing the precondition under which the function isn't supposed to
-  // initialize its out-parameter, and additionally check that such
-  // precondition can actually be fulfilled on the current path.
-  if (Call->isInSystemHeader()) {
-// We make an exception for system header functions that have no branches.
-// Such functions unconditionally fail to initialize the variable.
-// If they call other functions that have more paths within them,
-// this suppression would still apply when we visit these inner functions.
-// One common example of a standard function that doesn't ever initialize
-// its out parameter is operator placement new; it's up to the follow-up
-// constructor (if any) to initialize the memory.
-if (!N->getStackFrame()->getCFG()->isLinear()) {
-  static int i = 0;
-  R.markInvalid(&i, nullptr);
-}
+  if (Call->isInSystemHeader())
 return nullptr;
-  }
 
   if (const auto *MC = dyn_cast(Call)) {
 // If we failed to construct a piece for self, we still want to check
@@ -492,6 +471,70 @@
   return maybeEmitNoteForParameters(R, *Call, N);
 }
 
+//===--===//
+// Implementation of SuppressSystemHeaderWarningVistor.
+//===--===//
+
+// This visitor suppresses warnings coming from inlined system
+// headers functions when we exit the call that have no branches.
+// This is a very primitive visitor. It flat-out suppresses every report that
+// have a node that matches all the conditions above.
+// It should be used carefully!
+//
+// A good example for the usage of this visitor is when we want to suppress
+// warnings when a system header function does not initialize their arguments.
+// It's too unlikely a system header's fault.
+// It's much more likely a situation in which the function has a failure
+// mode that the user decided not to check. If we want to hunt such
+// omitted checks, we should provide an explicit function-specific note
+// describing the precondition under which the function isn't supposed to
+// initialize its out-parameter, and additionally check that such
+// precondition can actually be fulfilled on the current path.
+//
+// System header functions that have no branches unconditionally fail to
+// initialize the variable.
+// If they call other functions that have more paths within them,
+// this suppression would still apply when we visit these inner functions.
+// One common example of a standard function that doesn't ever initialize
+// its out parameter is operator placement new; it's up to the follow-up
+// constructor (if any) to initialize the memory
+namespace {
+class SuppressSystemHeaderWarningVisitor : public BugReporterVisitor {
+public:
+  virtual PathDiagnosticPieceRef VisitNode(const ExplodedNode *,
+   BugReporterContext &,
+   PathSensitiveBugReport &) override;
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+static int Tag = 0;
+ID.AddPointer(&Tag);
+  }
+};
+} // namespace
+
+PathDiagnosticPieceRef
+SuppressSystemHeaderWarningVisitor::VisitNode(const ExplodedNode *Succ,
+  BugReporterContext &BRC,
+  PathSensitiveBugReport &BR) {
+  const LocationContext *Ctx = Succ->getLocationContext();
+  const StackFrameContext *SCtx = Ctx->getStackFrame();
+  ProgramStateRef State = Succ->getState();
+  auto CallExitLoc = Succ->getLocationAs();
+
+  if (!CallExitLoc)
+return nullptr;
+
+  CallEventRef<> Call =
+  BRC.getStateManager().getCallEventManager().getCaller(SCtx, State);
+
+