[PATCH] D104036: [clang][deps] Prevent unintended modifications of the original TU command-line

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D104036#2816602 , @thakis wrote:

> Either this or your concurrent commit broke check-clang: 
> http://45.33.8.238/linux/48839/step_7.txt
>
> Please take a look and revert for now if it takes a while to fix.

This test has been intermittently failing since yesterday: 
https://lab.llvm.org/buildbot/#/builders/109/builds/16653
I've notified the author here: 
https://reviews.llvm.org/rG673c5ba58497298a684f8b8dfddbfb11cd89950e


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104036

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

[PATCH] D104036: [clang][deps] Prevent unintended modifications of the original TU command-line

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Either this or your concurrent commit broke check-clang: 
http://45.33.8.238/linux/48839/step_7.txt

Please take a look and revert for now if it takes a while to fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104036

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

[PATCH] D104036: [clang][deps] Prevent unintended modifications of the original TU command-line

[Jan Svoboda via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG80c0c639687e: [clang][deps] Prevent unintended modifications 
of the original TU command-line (authored by jansvoboda11).

Changed prior to commit:
  https://reviews.llvm.org/D104036?vs=351174=351833#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104036

Files:
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/Inputs/preserved-args/cdb.json.template
  clang/test/ClangScanDeps/Inputs/preserved-args/mod.h
  clang/test/ClangScanDeps/Inputs/preserved-args/module.modulemap
  clang/test/ClangScanDeps/Inputs/preserved-args/tu.c
  clang/test/ClangScanDeps/preserved-args.c

Index: clang/test/ClangScanDeps/preserved-args.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/preserved-args.c
@@ -0,0 +1,26 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: cp -r %S/Inputs/preserved-args/* %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: echo -%t > %t/result.json
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full >> %t/result.json
+// RUN: cat %t/result.json | sed 's:\?:/:g' | FileCheck %s
+
+// CHECK:  -[[PREFIX:.*]]
+// CHECK-NEXT: {
+// CHECK-NEXT:   "modules": [
+// CHECK-NEXT: {
+// CHECK:"command-line": [
+// CHECK-NEXT: "-cc1"
+// CHECK:  "-serialize-diagnostic-file"
+// CHECK-NEXT: "[[PREFIX]]/tu.dia"
+// CHECK:  "-fmodule-file=Foo=[[PREFIX]]/foo.pcm"
+// CHECK:  "-MT"
+// CHECK-NEXT: "my_target"
+// CHECK:  "-dependency-file"
+// CHECK-NEXT: "[[PREFIX]]/tu.d"
+// CHECK:],
+// CHECK:"name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK:  }
Index: clang/test/ClangScanDeps/Inputs/preserved-args/tu.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/preserved-args/tu.c
@@ -0,0 +1 @@
+#include "mod.h"
Index: clang/test/ClangScanDeps/Inputs/preserved-args/module.modulemap
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/preserved-args/module.modulemap
@@ -0,0 +1,3 @@
+module Mod {
+  header "mod.h"
+}
Index: clang/test/ClangScanDeps/Inputs/preserved-args/mod.h
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/preserved-args/mod.h
@@ -0,0 +1 @@
+// mod.h
Index: clang/test/ClangScanDeps/Inputs/preserved-args/cdb.json.template
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/preserved-args/cdb.json.template
@@ -0,0 +1,7 @@
+[
+  {
+"directory": "DIR",
+"command": "clang -MD -MT my_target -serialize-diagnostics DIR/tu.dia -fsyntax-only DIR/tu.c -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache -fmodule-file=Foo=DIR/foo.pcm -o DIR/tu.o",
+"file": "DIR/tu.c"
+  }
+]
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -20,8 +20,8 @@
 
 CompilerInvocation ModuleDepCollector::makeInvocationForModuleBuildWithoutPaths(
 const ModuleDeps ) const {
-  // Make a deep copy of the invocation.
-  CompilerInvocation CI(Instance.getInvocation());
+  // Make a deep copy of the original Clang invocation.
+  CompilerInvocation CI(OriginalInvocation);
 
   // Remove options incompatible with explicit module build.
   CI.getFrontendOpts().Inputs.clear();
@@ -39,9 +39,6 @@
 CI.getFrontendOpts().ModuleMapFiles.push_back(PrebuiltModule.ModuleMapFile);
   }
 
-  // Restore the original set of prebuilt module files.
-  CI.getHeaderSearchOpts().PrebuiltModuleFiles = OriginalPrebuiltModuleFiles;
-
   CI.getPreprocessorOpts().ImplicitPCHInclude.clear();
 
   return CI;
@@ -269,10 +266,9 @@
 
 ModuleDepCollector::ModuleDepCollector(
 std::unique_ptr Opts, CompilerInstance ,
-DependencyConsumer ,
-std::map> OriginalPrebuiltModuleFiles)
+DependencyConsumer , CompilerInvocation &)
 : Instance(I), Consumer(C), Opts(std::move(Opts)),
-  OriginalPrebuiltModuleFiles(std::move(OriginalPrebuiltModuleFiles)) {}
+  OriginalInvocation(std::move(OriginalCI)) {}
 
 void ModuleDepCollector::attachToPreprocessor(Preprocessor ) {
   PP.addPPCallbacks(std::make_unique(Instance, *this));
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

[PATCH] D104036: [clang][deps] Prevent unintended modifications of the original TU command-line

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:194
  CompilerInstance , DependencyConsumer ,
- std::map>
- OriginalPrebuiltModuleFiles);
+ CompilerInvocation OriginalInvocation);
 

dexonsmith wrote:
> I wonder if it'd be better to take `CompilerInvocation&&` here. Then the 
> caller is required to either pass `std::move` or make a deep copy at the call 
> site, and it's perhaps more clear that there's a deep copy being made.
That's much clearer indeed. Thanks for the suggestion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104036

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

[PATCH] D104036: [clang][deps] Prevent unintended modifications of the original TU command-line

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM, with one suggestion inline.




Comment at: 
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:194
  CompilerInstance , DependencyConsumer ,
- std::map>
- OriginalPrebuiltModuleFiles);
+ CompilerInvocation OriginalInvocation);
 

I wonder if it'd be better to take `CompilerInvocation&&` here. Then the caller 
is required to either pass `std::move` or make a deep copy at the call site, 
and it's perhaps more clear that there's a deep copy being made.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104036

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

[PATCH] D104036: [clang][deps] Prevent unintended modifications of the original TU command-line

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, dexonsmith.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

One of the goals of the dependency scanner is to provide command-lines that can 
be used to build modular dependencies of a translation unit. The only 
modifications to these command-lines should be for the purposes of explicit 
modular build.

The current version of dependency scanner leaks its implementation details into 
the command-lines for modular dependencies.

The first problem is that the `clang-scan-deps` tool adjusts the original 
textual command-line (adding `-Eonly -M -MT  -sys-header-deps 
-Wno-error -o /dev/null `, removing `--serialize-diagnostics`) in order to set 
up the `DependencyScanning` library. This has been addressed in D103461 
, D104012 , 
D104030 , D104031 
, D104033  
and moved into the library which directly adjusts a `CompilerInvocation` 
instead.

The `DependencyScanning` library now received the unmodifies 
`CompilerInvocation`, sets it up internally and uses it for the implicit build.

To prevent leaking the implementation details to the resulting command-lines 
(which get generated from this `CompilerInvocation`), this patch immediately 
makes a deep copy of the original invocation, stores the copy and uses it to 
generate the command-lines instead of the modified one.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104036

Files:
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/Inputs/preserved-args/cdb.json.template
  clang/test/ClangScanDeps/Inputs/preserved-args/mod.h
  clang/test/ClangScanDeps/Inputs/preserved-args/module.modulemap
  clang/test/ClangScanDeps/Inputs/preserved-args/tu.c
  clang/test/ClangScanDeps/preserved-args.c

Index: clang/test/ClangScanDeps/preserved-args.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/preserved-args.c
@@ -0,0 +1,26 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: cp -r %S/Inputs/preserved-args/* %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: echo -%t > %t/result.json
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full >> %t/result.json
+// RUN: cat %t/result.json | FileCheck %s
+
+// CHECK:  -[[PREFIX:.*]]
+// CHECK-NEXT: {
+// CHECK-NEXT:   "modules": [
+// CHECK-NEXT: {
+// CHECK:"command-line": [
+// CHECK-NEXT: "-cc1"
+// CHECK:  "-serialize-diagnostic-file"
+// CHECK-NEXT: "[[PREFIX]]/tu.dia"
+// CHECK:  "-fmodule-file=Foo=[[PREFIX]]/foo.pcm"
+// CHECK:  "-MT"
+// CHECK-NEXT: "my_target"
+// CHECK:  "-dependency-file"
+// CHECK-NEXT: "[[PREFIX]]/tu.d"
+// CHECK:],
+// CHECK:"name": "Mod"
+// CHECK-NEXT: }
+// CHECK-NEXT:   ]
+// CHECK:  }
Index: clang/test/ClangScanDeps/Inputs/preserved-args/tu.c
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/preserved-args/tu.c
@@ -0,0 +1 @@
+#include "mod.h"
Index: clang/test/ClangScanDeps/Inputs/preserved-args/module.modulemap
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/preserved-args/module.modulemap
@@ -0,0 +1,3 @@
+module Mod {
+  header "mod.h"
+}
Index: clang/test/ClangScanDeps/Inputs/preserved-args/mod.h
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/preserved-args/mod.h
@@ -0,0 +1 @@
+// mod.h
Index: clang/test/ClangScanDeps/Inputs/preserved-args/cdb.json.template
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/preserved-args/cdb.json.template
@@ -0,0 +1,7 @@
+[
+  {
+"directory": "DIR",
+"command": "clang -MD -MT my_target -serialize-diagnostics DIR/tu.dia -fsyntax-only DIR/tu.c -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache -fmodule-file=Foo=DIR/foo.pcm -o DIR/tu.o",
+"file": "DIR/tu.c"
+  }
+]
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -20,8 +20,8 @@
 
 CompilerInvocation ModuleDepCollector::makeInvocationForModuleBuildWithoutPaths(
 const ModuleDeps ) const {
-  // Make a deep copy of the invocation.
-  CompilerInvocation