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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

While we were there, we also dug into `std::any`, and learned that the analyzer 
can model it shockingly well. Hopefully we can submit a few patches that 
demonstrates it in a form of some test files.

In D142354#4079758 , @steakhal wrote:

> I would be interested in some of the free-functions dealing with variants, 
> such as `std::visit()`. https://godbolt.org/z/bbocrz4dG
> I hope that's also on the radar.

Thank you so much!! We definitely intend to work on this issue, and haven't 
thought of it before your suggestion.


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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D142354#4079643 , @Szelethus wrote:

>> In the latter case, have you explored the possibility of force-inlining the 
>> class instead, like I suggested in the thread?
>
> I have done some tests then, and figured that the analyzer can't really 
> follow what happens in std::variant. I admit, now that I've taken a more 
> thorough look, I was wrong! Here are some examples.
>
>   std::variant v = 0;
>   int i = std::get(v);
>   clang_analyzer_eval(i == 0); // expected-warning{{TRUE}}
>   10 / i; // FIXME: This should warn for division by zero!
>
>
>
>   std::variant v = 0;
>   std::string *sptr = std::get_if();
>   clang_analyzer_eval(sptr == nullptr); // expected-warning{{TRUE}}
>   *sptr = "Alma!"; // FIXME: Should warn, sptr is a null pointer!
>
>
>
>   void g(std::variant v) {
> if (!std::get_if())
>   return;
> int *iptr = std::get_if();
> clang_analyzer_eval(iptr == nullptr); // expected-warning{{TRUE}}
> *iptr = 5; // FIXME: Should warn, iptr is a null pointer!
>   }
>
> In neither of these cases was a warning emitted, but that was a result of bug 
> report suppression, because the exploded graph indicates that the errors were 
> found.
>
> We will need to teach these suppression strategies in these cases, the 
> heuristic is too conservative, and I wouldn't mind some `NoteTag`s to tell 
> the user something along the lines of "Assuming this variant holds and 
> std::string value".

Yeah, right, these are probably inlined defensive check suppressions (type-2, 
the ones with "let's early-return null"). So you'll need to see how these 
inlined stack frames look like and what makes them different from the unwanted 
case. (And it's possible that there are no formal differences, other than the 
programmer's "intention"; but it could also be something really simple).

So in any case, I'm really curious whether there's any solution besides "let's 
model everything manually", given that we've spent two summers modeling 
`unique_ptr` manually and never finished it.


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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I would be interested in some of the free-functions dealing with variants, such 
as `std::visit()`. https://godbolt.org/z/bbocrz4dG
I hope that's also on the radar.

In D142354#4079643 , @Szelethus wrote:

> In D142354#4078450 , @NoQ wrote:
>
>> Have you found a reasonable amount of code that uses `std::variant`, to test 
>> your checker on?
>
> Acid  has a few, csv-parser 
>  in one place, there is a couple in 
> config-loader  and cista 
> , and a surprising amount in userver 
> . Though its a question how 
> painful it is to set up their dependencies.

I think bitcoin  and qtbase 
 also have some uses for variant.


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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D142354#4078450 , @NoQ wrote:

> Interesting, what specific goals do you have here? Are you planning to find 
> specific bugs (eg. force-unwrap to a wrong type) or just to model the 
> semantics?

Hi!

Meant to write this comment yesterday, but here we go. My idea was aiming for 
both of the goals you mentioned:

1. Emit diagnostics on improper usages of `std::variant`, which mostly boils 
down retrieving a field through a wrong type, yes. This will be, in my view, 
the main showpiece of the thesis.
2. Model the semantics.

In this or in a followup patch I think we should demonstrate with a few tests 
what we expect the checker to be capable of.

> In the latter case, have you explored the possibility of force-inlining the 
> class instead, like I suggested in the thread?

I have done some tests then, and figured that the analyzer can't really follow 
what happens in std::variant. I admit, now that I've taken a more thorough 
look, I was wrong! Here are some examples.

  std::variant v = 0;
  int i = std::get(v);
  clang_analyzer_eval(i == 0); // expected-warning{{TRUE}}
  10 / i; // FIXME: This should warn for division by zero!



  std::variant v = 0;
  std::string *sptr = std::get_if();
  clang_analyzer_eval(sptr == nullptr); // expected-warning{{TRUE}}
  *sptr = "Alma!"; // FIXME: Should warn, sptr is a null pointer!



  void g(std::variant v) {
if (!std::get_if())
  return;
int *iptr = std::get_if();
clang_analyzer_eval(iptr == nullptr); // expected-warning{{TRUE}}
*iptr = 5; // FIXME: Should warn, iptr is a null pointer!
  }

In neither of these cases was a warning emitted, but that was a result of bug 
report suppression, because the exploded graph indicates that the errors were 
found.

We will need to teach these suppression strategies in these cases, the 
heuristic is too conservative, and I wouldn't mind some `NoteTag`s to tell the 
user something along the lines of "Assuming this variant holds and std::string 
value".

> Have you found a reasonable amount of code that uses `std::variant`, to test 
> your checker on?

Acid  has a few, csv-parser 
 in one place, there is a couple in 
config-loader  and cista 
, and a surprising amount in userver 
. Though its a question how 
painful it is to set up their dependencies.


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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Interesting, what specific goals do you have here? Are you planning to find 
specific bugs (eg. force-unwrap to a wrong type) or just to model the 
semantics? In the latter case, have you explored the possibility of 
force-inlining the class instead, like I suggested in the thread? Have you 
found a reasonable amount of code that uses `std::variant`, to test your 
checker on?




Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:312
+
 } // end "alpha.core"
 

Maybe `alpha.cplusplus`?


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 , CheckerContext ) 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 ) {
+  return true;
+}
+
+void clang::ento::registerStdVariantChecker(clang::ento::CheckerManager ) {
+  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 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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I'm very much interrested. I think you should define a mock standard variant 
header, that you could include from your tests.


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 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 , CheckerContext ) 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 ) {
+  return true;
+}
+
+void clang::ento::registerStdVariantChecker(clang::ento::CheckerManager ) {
+  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]}}
+}
\ No newline at