[clang][dataflow] Allow DataflowAnalysisContext to use a non-owned Solver.

[martinboehme]

For some callers (see change in DataflowAnalysis.h), this is more convenient.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

@llvm/pr-subscribers-clang

Author: None (martinboehme)

Changes

For some callers (see change in DataflowAnalysis.h), this is more convenient.

---

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

3 Files Affected:

• (modified) clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h (+2-3)
• (modified) clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h (+18-2)
• (modified) clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp (+5-5)

diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
index 67eccdd030dcd..763af24454764 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
@@ -283,9 +283,8 @@ llvm::Expected<llvm::SmallVector<Diagnostic>> diagnoseFunction(
   if (!Context)
     return Context.takeError();
 
-  auto OwnedSolver = std::make_unique<WatchedLiteralsSolver>(MaxSATIterations);
-  const WatchedLiteralsSolver *Solver = OwnedSolver.get();
-  DataflowAnalysisContext AnalysisContext(std::move(OwnedSolver));
+  auto Solver = std::make_unique<WatchedLiteralsSolver>(MaxSATIterations);
+  DataflowAnalysisContext AnalysisContext(*Solver);
   Environment Env(AnalysisContext, FuncDecl);
   AnalysisT Analysis = createAnalysis<AnalysisT>(ASTCtx, Env);
   llvm::SmallVector<Diagnostic> Diagnostics;
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
index aa2c366cb164a..13a74281e02b5 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
@@ -67,7 +67,19 @@ class DataflowAnalysisContext {
   DataflowAnalysisContext(std::unique_ptr<Solver> S,
                           Options Opts = Options{
                               /*ContextSensitiveOpts=*/std::nullopt,
-                              /*Logger=*/nullptr});
+                              /*Logger=*/nullptr})
+      : DataflowAnalysisContext(*S, std::move(S), Opts) {}
+
+  /// Constructs a dataflow analysis context.
+  ///
+  /// Requirements:
+  ///
+  ///  `S` must outlive the `DataflowAnalysisContext`.
+  DataflowAnalysisContext(Solver &S, Options Opts = Options{
+                                         /*ContextSensitiveOpts=*/std::nullopt,
+                                         /*Logger=*/nullptr})
+      : DataflowAnalysisContext(S, nullptr, Opts) {}
+
   ~DataflowAnalysisContext();
 
   /// Sets a callback that returns the names and types of the synthetic fields
@@ -209,6 +221,9 @@ class DataflowAnalysisContext {
     using DenseMapInfo::isEqual;
   };
 
+  DataflowAnalysisContext(Solver &S, std::unique_ptr<Solver> OwnedSolver,
+                          Options Opts);
+
   // Extends the set of modeled field declarations.
   void addModeledFields(const FieldSet &Fields);
 
@@ -232,7 +247,8 @@ class DataflowAnalysisContext {
            Solver::Result::Status::Unsatisfiable;
   }
 
-  std::unique_ptr<Solver> S;
+  Solver &S;
+  std::unique_ptr<Solver> OwnedSolver;
   std::unique_ptr<Arena> A;
 
   // Maps from program declarations and statements to storage locations that are
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
index e94fd39c45dc1..3041dcf6d500c 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
@@ -170,7 +170,7 @@ DataflowAnalysisContext::joinFlowConditions(Atom FirstToken,
 
 Solver::Result DataflowAnalysisContext::querySolver(
     llvm::SetVector<const Formula *> Constraints) {
-  return S->solve(Constraints.getArrayRef());
+  return S.solve(Constraints.getArrayRef());
 }
 
 bool DataflowAnalysisContext::flowConditionImplies(Atom Token,
@@ -338,10 +338,10 @@ static std::unique_ptr<Logger> makeLoggerFromCommandLine() {
   return Logger::html(std::move(StreamFactory));
 }
 
-DataflowAnalysisContext::DataflowAnalysisContext(std::unique_ptr<Solver> S,
-                                                 Options Opts)
-    : S(std::move(S)), A(std::make_unique<Arena>()), Opts(Opts) {
-  assert(this->S != nullptr);
+DataflowAnalysisContext::DataflowAnalysisContext(
+    Solver &S, std::unique_ptr<Solver> OwnedSolver, Options Opts)
+    : S(S), OwnedSolver(std::move(OwnedSolver)), A(std::make_unique<Arena>()),
+      Opts(Opts) {
   // If the -dataflow-log command-line flag was set, synthesize a logger.
   // This is ugly but provides a uniform method for ad-hoc debugging dataflow-
   // based tools.

[ymand]

Please add comments either here or on the fields relating S and OwnedSolver.

[martinboehme]

Done.

[martinboehme]

@ymand PTAL -- I noticed that my initial version had a use-after-move on this line:

  DataflowAnalysisContext(std::unique_ptr<Solver> S,
                          Options Opts = Options{
                              /*ContextSensitiveOpts=*/std::nullopt,
                              /*Logger=*/nullptr})
      : DataflowAnalysisContext(*S, std::move(S), Opts) {}

This was because the private constructor to which this constructor delegates originally took the std::unique_ptr<Solver> by value, and the order in which the arguments are evaluated is indeterminate.

I've now changed the parameter of the private constructor to be an rvalue reference (std::unique_ptr<Solver> &&), which resolves this issue as the move only happens inside the constructor.

[martinboehme]

CI failure looks unrelated.

[joker-eph]

I am not sure that the CI failure was unrelated actually: the CI is consistently broken after this patch (you have got an email about it?), I'll revert for now.

[joker-eph]

Actually your premerge failure here was an infra issue, but the CI is still broken after this PR somehow: https://lab.llvm.org/buildbot/#/builders/271/builds/7420

# .---command stderr------------
# | error: 'expected-error' diagnostics seen but not expected: 
# |   (frontend): '-fsyntax-only' action ignored; '-ast-print' action specified previously
# `-----------------------------

[martinboehme]

Actually your premerge failure here was an infra issue, but the CI is still broken after this PR somehow: https://lab.llvm.org/buildbot/#/builders/271/builds/7420

# .---command stderr------------
# | error: 'expected-error' diagnostics seen but not expected: 
# |   (frontend): '-fsyntax-only' action ignored; '-ast-print' action specified previously
# `-----------------------------

This is a failure in the test clang\test\OpenMP\target_ast_print.cpp, correct?

Are you sure this PR is the culprit? It touches only the dataflow framework, which is not used in OpenMP or the Clang compiler itself.

[martinboehme]

@joker-eph

PS I don't see a revert of this PR, but I do see a revert of a different PR here, and that PR does look as if it could be the culprit.

Maybe you commented on this PR by mistake?