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

[spaits]

Summary

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.

[steakhal]

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

[spaits]

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.

[spaits]

Adding mock header for std::variant.

[NoQ]

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?

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

312

Maybe alpha.cplusplus?

[Szelethus]

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<int, std::string> v = 0;
int i = std::get<int>(v);
clang_analyzer_eval(i == 0); // expected-warning{{TRUE}}
10 / i; // FIXME: This should warn for division by zero!

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

void g(std::variant<int, std::string> v) {
  if (!std::get_if<std::string>(&v))
    return;
  int *iptr = std::get_if<int>(&v);
  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 NoteTags 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.

[steakhal]

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.

[NoQ]

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<int, std::string> v = 0;
int i = std::get<int>(v);
clang_analyzer_eval(i == 0); // expected-warning{{TRUE}}
10 / i; // FIXME: This should warn for division by zero!

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

void g(std::variant<int, std::string> v) {
  if (!std::get_if<std::string>(&v))
    return;
  int *iptr = std::get_if<int>(&v);
  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 NoteTags 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.

[Szelethus]

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.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

def PthreadLockBase : Checker<"PthreadLockBase">,
   Documentation<NotDocumented>,
   Hidden;
 
 def C11LockChecker : Checker<"C11Lock">,
   HelpText<"Simple lock -> unlock checker">,
   Dependencies<[PthreadLockBase]>,
   Documentation<HasDocumentation>;
 
+def StdVariantChecker : Checker<"StdVariant">,
+  HelpText<"Check std::variant">,
+  Documentation<NotDocumented>;
+
 } // end "alpha.core"
 
 //===----------------------------------------------------------------------===//
 // Nullability checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = Nullability in {
 
 def NullabilityBase : Checker<"NullabilityBase">,

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   ReturnUndefChecker.cpp
   ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrChecker.cpp
   SmartPtrModeling.cpp
   StackAddrEscapeChecker.cpp
   StdLibraryFunctionsChecker.cpp
+  StdVariantChecker.cpp
   STLAlgorithmModeling.cpp
   StreamChecker.cpp
   StringChecker.cpp
   Taint.cpp
   TaintTesterChecker.cpp
   TestAfterDivZeroChecker.cpp
   TraversalChecker.cpp
   TrustNonnullChecker.cpp

clang/lib/StaticAnalyzer/Checkers/StdVariantChecker.cpp

+//===- 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<check::PreCall> {
+    CallDescription VariantConstructorCall{{"std", "variant"}, 0, 0};
+    BugType VariantCreated{this, "VariantCreated", "VariantCreated"};
+
+    public:
+    void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
+      if (!isa<CXXConstructorCall>(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<PathSensitiveBugReport>(
+          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<StdVariantChecker>();
+}
 No newline at end of file

clang/test/Analysis/Inputs/variant.h

+// 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... Types>
+  class variant {};
+} //end of namespace std
+
+#endif //VARIANT_H
 No newline at end of file

clang/test/Analysis/std-variant-checker.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.StdVariant %s -verify
+
+#include "Inputs/variant.h"
+
+void g() {
+  std::variant<int, char> v; // expected-warning{{Variant Created [alpha.core.StdVariant]}}
+}
 No newline at end of file