[clang][analyzer] Delay checking the model-path

[jansvoboda11]

This PR is part of an effort to remove file system usage from the command line parsing code. The reason for that is that it's impossible to do file system access correctly without a configured VFS, and the VFS can only be configured after the command line is parsed. I don't want to intertwine command line parsing and VFS configuration, so I decided to perform the file system access after the command line is parsed and the VFS is configured - ideally right before the file system entity is used for the first time.

This patch delays checking that model-path is an existing directory.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jan Svoboda (jansvoboda11)

Changes

This PR is part of an effort to remove file system usage from the command line parsing code. The reason for that is that it's impossible to do file system access correctly without a configured VFS, and the VFS can only be configured after the command line is parsed. I don't want to intertwine command line parsing and VFS configuration, so I decided to perform the file system access after the command line is parsed and the VFS is configured - ideally right before the file system entity is used for the first time.

This patch delays checking that model-path is an existing directory.

---

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

3 Files Affected:

• (modified) clang/lib/Frontend/CompilerInvocation.cpp (-5)
• (modified) clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp (+9-1)
• (added) clang/test/Analysis/model-file-missing.cpp (+3)

diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 3a36250da57a3..802b83ec25d70 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -1319,11 +1319,6 @@ static void parseAnalyzerConfigs(AnalyzerOptions &AnOpts,
   if (!AnOpts.CTUDir.empty() && !llvm::sys::fs::is_directory(AnOpts.CTUDir))
     Diags->Report(diag::err_analyzer_config_invalid_input) << "ctu-dir"
                                                            << "a filename";
-
-  if (!AnOpts.ModelPath.empty() &&
-      !llvm::sys::fs::is_directory(AnOpts.ModelPath))
-    Diags->Report(diag::err_analyzer_config_invalid_input) << "model-path"
-                                                           << "a filename";
 }
 
 /// Generate a remark argument. This is an inverse of `ParseOptimizationRemark`.
diff --git a/clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp b/clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
index 5d392afcb9825..011916830561d 100644
--- a/clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
+++ b/clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
@@ -11,6 +11,7 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/Basic/LangStandard.h"
 #include "clang/Basic/Stack.h"
+#include "clang/Basic/DiagnosticDriver.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendAction.h"
@@ -24,7 +25,14 @@
 using namespace clang;
 using namespace ento;
 
-ModelInjector::ModelInjector(CompilerInstance &CI) : CI(CI) {}
+ModelInjector::ModelInjector(CompilerInstance &CI) : CI(CI) {
+  if (!CI.getAnalyzerOpts().ModelPath.empty()) {
+    auto S = CI.getVirtualFileSystem().status(CI.getAnalyzerOpts().ModelPath);
+    if (!S || S->getType() != llvm::sys::fs::file_type::directory_file)
+      CI.getDiagnostics().Report(diag::err_analyzer_config_invalid_input)
+          << "model-path" << "a filename";
+  }
+}
 
 Stmt *ModelInjector::getBody(const FunctionDecl *D) {
   onBodySynthesis(D);
diff --git a/clang/test/Analysis/model-file-missing.cpp b/clang/test/Analysis/model-file-missing.cpp
new file mode 100644
index 0000000000000..c9dfb4ec1b244
--- /dev/null
+++ b/clang/test/Analysis/model-file-missing.cpp
@@ -0,0 +1,3 @@
+// RUN: not %clang_analyze_cc1 -analyzer-checker=core -analyzer-config model-path=%t/blah %s -o - 2>&1 | FileCheck %s
+// CHECK: error: invalid input for analyzer-config option 'model-path', that expects a filename value
+// CHECK-NEXT: 1 error generated

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Jan Svoboda (jansvoboda11)

Changes

This PR is part of an effort to remove file system usage from the command line parsing code. The reason for that is that it's impossible to do file system access correctly without a configured VFS, and the VFS can only be configured after the command line is parsed. I don't want to intertwine command line parsing and VFS configuration, so I decided to perform the file system access after the command line is parsed and the VFS is configured - ideally right before the file system entity is used for the first time.

This patch delays checking that model-path is an existing directory.

---

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

3 Files Affected:

• (modified) clang/lib/Frontend/CompilerInvocation.cpp (-5)
• (modified) clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp (+9-1)
• (added) clang/test/Analysis/model-file-missing.cpp (+3)

diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 3a36250da57a3..802b83ec25d70 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -1319,11 +1319,6 @@ static void parseAnalyzerConfigs(AnalyzerOptions &AnOpts,
   if (!AnOpts.CTUDir.empty() && !llvm::sys::fs::is_directory(AnOpts.CTUDir))
     Diags->Report(diag::err_analyzer_config_invalid_input) << "ctu-dir"
                                                            << "a filename";
-
-  if (!AnOpts.ModelPath.empty() &&
-      !llvm::sys::fs::is_directory(AnOpts.ModelPath))
-    Diags->Report(diag::err_analyzer_config_invalid_input) << "model-path"
-                                                           << "a filename";
 }
 
 /// Generate a remark argument. This is an inverse of `ParseOptimizationRemark`.
diff --git a/clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp b/clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
index 5d392afcb9825..011916830561d 100644
--- a/clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
+++ b/clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
@@ -11,6 +11,7 @@
 #include "clang/AST/DeclObjC.h"
 #include "clang/Basic/LangStandard.h"
 #include "clang/Basic/Stack.h"
+#include "clang/Basic/DiagnosticDriver.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendAction.h"
@@ -24,7 +25,14 @@
 using namespace clang;
 using namespace ento;
 
-ModelInjector::ModelInjector(CompilerInstance &CI) : CI(CI) {}
+ModelInjector::ModelInjector(CompilerInstance &CI) : CI(CI) {
+  if (!CI.getAnalyzerOpts().ModelPath.empty()) {
+    auto S = CI.getVirtualFileSystem().status(CI.getAnalyzerOpts().ModelPath);
+    if (!S || S->getType() != llvm::sys::fs::file_type::directory_file)
+      CI.getDiagnostics().Report(diag::err_analyzer_config_invalid_input)
+          << "model-path" << "a filename";
+  }
+}
 
 Stmt *ModelInjector::getBody(const FunctionDecl *D) {
   onBodySynthesis(D);
diff --git a/clang/test/Analysis/model-file-missing.cpp b/clang/test/Analysis/model-file-missing.cpp
new file mode 100644
index 0000000000000..c9dfb4ec1b244
--- /dev/null
+++ b/clang/test/Analysis/model-file-missing.cpp
@@ -0,0 +1,3 @@
+// RUN: not %clang_analyze_cc1 -analyzer-checker=core -analyzer-config model-path=%t/blah %s -o - 2>&1 | FileCheck %s
+// CHECK: error: invalid input for analyzer-config option 'model-path', that expects a filename value
+// CHECK-NEXT: 1 error generated

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[Xazax-hun]

While this change looks good to me now this file system related validations will be scattered all over the codebase. I don't have a strong opinion but wondering if having a validation step right after the command line parsing is done would be better.

Delaying some of these validations for too long can make them more lazy. E.g., if someone appends some command line flags to only do preprocessing, we might end up not doing these validations at all. There are some pros and cons to this. Clang becomes more lenient about mistakes in the command line flags but this might bite users later. When they remove/add an unrelated flag new diagnostics might pop up.

[jansvoboda11]

I considered adding a separate step to perform the FS checks, but decided against it. There are lots of clients of the command line parsing code, and adding a new required step is a large breaking change and it would be easy to miss in new code. I think the next best place for these checks is near the code that actually uses the paths.