[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir 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 rGf80a0ea76072: [clang][deps] Split translation units into 
individual -cc1 or other commands (authored by benlangmuir).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132405

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/deprecated-driver-api.c
  clang/test/ClangScanDeps/diagnostics.c
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/ClangScanDeps/modules-context-hash-ignore-macros.c
  clang/test/ClangScanDeps/modules-context-hash-outputs.c
  clang/test/ClangScanDeps/modules-context-hash-warnings.c
  clang/test/ClangScanDeps/modules-context-hash.c
  clang/test/ClangScanDeps/modules-dep-args.c
  clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
  clang/test/ClangScanDeps/modules-full.cpp
  clang/test/ClangScanDeps/modules-implicit-dot-private.m
  clang/test/ClangScanDeps/modules-incomplete-umbrella.c
  clang/test/ClangScanDeps/modules-inferred.m
  clang/test/ClangScanDeps/modules-no-undeclared-includes.c
  clang/test/ClangScanDeps/modules-pch-common-submodule.c
  clang/test/ClangScanDeps/modules-pch-common-via-submodule.c
  clang/test/ClangScanDeps/modules-pch.c
  clang/test/ClangScanDeps/multiple-commands.c
  clang/test/ClangScanDeps/removed-args.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp
  clang/utils/module-deps-to-rsp.py

Index: clang/utils/module-deps-to-rsp.py
===
--- clang/utils/module-deps-to-rsp.py
+++ clang/utils/module-deps-to-rsp.py
@@ -48,6 +48,9 @@
   type=str)
   action.add_argument("--tu-index", help="The index of the translation unit to get arguments for",
   type=int)
+  parser.add_argument("--tu-cmd-index",
+  help="The index of the command within the translation unit (default=0)",
+  type=int, default=0)
   args = parser.parse_args()
 
   full_deps = parseFullDeps(json.load(open(args.full_deps_file, 'r')))
@@ -58,7 +61,8 @@
 if args.module_name:
   cmd = findModule(args.module_name, full_deps)['command-line']
 elif args.tu_index != None:
-  cmd = full_deps.translation_units[args.tu_index]['command-line']
+  tu = full_deps.translation_units[args.tu_index]
+  cmd = tu['commands'][args.tu_cmd_index]['command-line']
 
 print(" ".join(map(quote, cmd)))
   except:
Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -182,6 +182,11 @@
 llvm::cl::desc("The names of dependency targets for the dependency file"),
 llvm::cl::cat(DependencyScannerCategory));
 
+llvm::cl::opt DeprecatedDriverCommand(
+"deprecated-driver-command", llvm::cl::Optional,
+llvm::cl::desc("use a single driver command to build the tu (deprecated)"),
+llvm::cl::cat(DependencyScannerCategory));
+
 enum ResourceDirRecipeKind {
   RDRK_ModifyCompilerPath,
   RDRK_InvokeCompiler,
@@ -256,7 +261,7 @@
 public:
   void mergeDeps(StringRef Input, FullDependenciesResult FDR,
  size_t InputIndex) {
-const FullDependencies  = FDR.FullDeps;
+FullDependencies  = FDR.FullDeps;
 
 InputDeps ID;
 ID.FileName = std::string(Input);
@@ -274,7 +279,8 @@
   Modules.insert(I, {{MD.ID, InputIndex}, std::move(MD)});
 }
 
-ID.CommandLine = FD.CommandLine;
+ID.DriverCommandLine = std::move(FD.DriverCommandLine);
+ID.Commands = std::move(FD.Commands);
 Inputs.push_back(std::move(ID));
   }
 
@@ -311,14 +317,33 @@
 
 Array TUs;
 for (auto & : Inputs) {
-  Object O{
-  {"input-file", I.FileName},
-  {"clang-context-hash", I.ContextHash},
-  {"file-deps", I.FileDeps},
-  {"clang-module-deps", toJSONSorted(I.ModuleDeps)},
-  {"command-line", I.CommandLine},
-  };
-  TUs.push_back(std::move(O));
+  Array Commands;
+  if (I.DriverCommandLine.empty()) {
+for (const auto  : I.Commands) {
+  Object O{
+  {"input-file", I.FileName},
+  {"clang-context-hash", I.ContextHash},
+  {"file-deps", I.FileDeps},
+  {"clang-module-deps", toJSONSorted(I.ModuleDeps)},
+  {"executable", Cmd.Executable},
+  {"command-line", Cmd.Arguments},
+ 

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir marked 2 inline comments as done.
benlangmuir added inline comments.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:295
+if (MDC)
+  MDC->applyDiscoveredDependencies(CI);
+LastCC1Arguments = CI.getCC1CommandLine();

jansvoboda11 wrote:
> I'm not a fan of storing `MDC` as a member, but I don't see any clearly 
> better alternatives. One option would be to pass `CompilerInvocation` 
> out-parameter to `ModuleDepCollector`'s constructor and relying on the 
> collector to apply discovered dependencies, but that's not super obvious.
We are applying changes to multiple CompilerInvocations (the scanned one, but 
also any later ones that are downstream of it), so it's less obvious to me how 
to do that here.  We would need to pass in all of the invocations, but 
currently only one of them is in-memory at a time.  I'm open to other ideas 
here, but I haven't come up with any great ideas.  Maybe there's a refactoring 
of MDC that splits the scanning state from the state necessary to apply changes 
to the CI so we would only expose the minimum information needed, but I expect 
that would be an invasive change so prefer not to attempt it in this patch.


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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Jan Svoboda via Phabricator via cfe-commits]

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

LGTM. Up to you if you act on the last comment. Thanks for seeing this through!




Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:295
+if (MDC)
+  MDC->applyDiscoveredDependencies(CI);
+LastCC1Arguments = CI.getCC1CommandLine();

I'm not a fan of storing `MDC` as a member, but I don't see any clearly better 
alternatives. One option would be to pass `CompilerInvocation` out-parameter to 
`ModuleDepCollector`'s constructor and relying on the collector to apply 
discovered dependencies, but that's not super obvious.


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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir marked 6 inline comments as done.
benlangmuir added inline comments.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:165
 
+if (Scanned) {
+  // If we have already scanned an upstream command, just forward to the

jansvoboda11 wrote:
> This makes sure we only run scan once per driver invocation? Can you expand 
> on this a bit? Maybe even put the reasoning into a comment in the code.
In theory you want to scan once for each independent chain of -cc1 commands, 
but since we don't yet support multi-arch builds in the scanner that just means 
scan once per driver invocation.

I'll add a comment.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:444
+  Invocation.setDiagnosticOptions(>getDiagnosticOptions());
+  return Invocation.run();
+});

jansvoboda11 wrote:
> I'm not particularly fond of the fact that `Consumer.handleBuildCommand()` is 
> called in this lambda directly in the non-clang case, but several objects 
> deep in the normal case (`ToolInvocation` -> `DependencyScanningAction`). I 
> think a clearer way to do this would be to somehow extract the 
> `CompilerInvocation` (or `Command`) result from `ToolInvocation` and report 
> it in this lambda too.
Yeah, this area has gone through a lot of churn as I try to balance the desire 
to keep it clear where the consumer should be called vs. trying to keep the MDC 
contained to the action.

I took another crack at it in the latest diff.



Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:177
 
+static bool needsModules(FrontendInputFile FIF) {
+  switch (FIF.getKind().getLanguage()) {

jansvoboda11 wrote:
> I think this could be useful for other tools too in the future. Do you think 
> it would make sense to put this in a more prominent header, so that other 
> people can find it and reuse it more easily?
I would prefer not to expose this without more understanding of what other use 
cases there are. It seems like there are many ways to interpret "needsModules" 
-- most of the time you probably want something more like 
`LangOptions::Modules`.


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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 455712.
benlangmuir added a comment.

- handleBuildCommand now takes a Command
- Removed now-unused forward decl
- Expanded comment about scan-once behaviour
- Moved all calls to handleBuildCommand to a single place
- Added comments to new tests


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

https://reviews.llvm.org/D132405

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/deprecated-driver-api.c
  clang/test/ClangScanDeps/diagnostics.c
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/ClangScanDeps/modules-context-hash-ignore-macros.c
  clang/test/ClangScanDeps/modules-context-hash-outputs.c
  clang/test/ClangScanDeps/modules-context-hash-warnings.c
  clang/test/ClangScanDeps/modules-context-hash.c
  clang/test/ClangScanDeps/modules-dep-args.c
  clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
  clang/test/ClangScanDeps/modules-full.cpp
  clang/test/ClangScanDeps/modules-implicit-dot-private.m
  clang/test/ClangScanDeps/modules-incomplete-umbrella.c
  clang/test/ClangScanDeps/modules-inferred.m
  clang/test/ClangScanDeps/modules-no-undeclared-includes.c
  clang/test/ClangScanDeps/modules-pch-common-submodule.c
  clang/test/ClangScanDeps/modules-pch-common-via-submodule.c
  clang/test/ClangScanDeps/modules-pch.c
  clang/test/ClangScanDeps/multiple-commands.c
  clang/test/ClangScanDeps/removed-args.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp
  clang/utils/module-deps-to-rsp.py

Index: clang/utils/module-deps-to-rsp.py
===
--- clang/utils/module-deps-to-rsp.py
+++ clang/utils/module-deps-to-rsp.py
@@ -48,6 +48,9 @@
   type=str)
   action.add_argument("--tu-index", help="The index of the translation unit to get arguments for",
   type=int)
+  parser.add_argument("--tu-cmd-index",
+  help="The index of the command within the translation unit (default=0)",
+  type=int, default=0)
   args = parser.parse_args()
 
   full_deps = parseFullDeps(json.load(open(args.full_deps_file, 'r')))
@@ -58,7 +61,8 @@
 if args.module_name:
   cmd = findModule(args.module_name, full_deps)['command-line']
 elif args.tu_index != None:
-  cmd = full_deps.translation_units[args.tu_index]['command-line']
+  tu = full_deps.translation_units[args.tu_index]
+  cmd = tu['commands'][args.tu_cmd_index]['command-line']
 
 print(" ".join(map(quote, cmd)))
   except:
Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -182,6 +182,11 @@
 llvm::cl::desc("The names of dependency targets for the dependency file"),
 llvm::cl::cat(DependencyScannerCategory));
 
+llvm::cl::opt DeprecatedDriverCommand(
+"deprecated-driver-command", llvm::cl::Optional,
+llvm::cl::desc("use a single driver command to build the tu (deprecated)"),
+llvm::cl::cat(DependencyScannerCategory));
+
 enum ResourceDirRecipeKind {
   RDRK_ModifyCompilerPath,
   RDRK_InvokeCompiler,
@@ -256,7 +261,7 @@
 public:
   void mergeDeps(StringRef Input, FullDependenciesResult FDR,
  size_t InputIndex) {
-const FullDependencies  = FDR.FullDeps;
+FullDependencies  = FDR.FullDeps;
 
 InputDeps ID;
 ID.FileName = std::string(Input);
@@ -274,7 +279,8 @@
   Modules.insert(I, {{MD.ID, InputIndex}, std::move(MD)});
 }
 
-ID.CommandLine = FD.CommandLine;
+ID.DriverCommandLine = std::move(FD.DriverCommandLine);
+ID.Commands = std::move(FD.Commands);
 Inputs.push_back(std::move(ID));
   }
 
@@ -311,14 +317,33 @@
 
 Array TUs;
 for (auto & : Inputs) {
-  Object O{
-  {"input-file", I.FileName},
-  {"clang-context-hash", I.ContextHash},
-  {"file-deps", I.FileDeps},
-  {"clang-module-deps", toJSONSorted(I.ModuleDeps)},
-  {"command-line", I.CommandLine},
-  };
-  TUs.push_back(std::move(O));
+  Array Commands;
+  if (I.DriverCommandLine.empty()) {
+for (const auto  : I.Commands) {
+  Object O{
+  {"input-file", I.FileName},
+  {"clang-context-hash", I.ContextHash},
+  {"file-deps", I.FileDeps},
+  {"clang-module-deps", toJSONSorted(I.ModuleDeps)},
+  {"executable", Cmd.Executable},
+  {"command-line", Cmd.Arguments},
+  };
+ 

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h:27
+namespace driver {
+class Command;
+}

Not needed anymore, I assume.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h:39
 
+  virtual void handleBuildCommand(std::string Executable,
+  std::vector Args) = 0;

Would it make sense to accept the `clang::tooling::dependencies::Command` 
struct here?



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:165
 
+if (Scanned) {
+  // If we have already scanned an upstream command, just forward to the

This makes sure we only run scan once per driver invocation? Can you expand on 
this a bit? Maybe even put the reasoning into a comment in the code.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:444
+  Invocation.setDiagnosticOptions(>getDiagnosticOptions());
+  return Invocation.run();
+});

I'm not particularly fond of the fact that `Consumer.handleBuildCommand()` is 
called in this lambda directly in the non-clang case, but several objects deep 
in the normal case (`ToolInvocation` -> `DependencyScanningAction`). I think a 
clearer way to do this would be to somehow extract the `CompilerInvocation` (or 
`Command`) result from `ToolInvocation` and report it in this lambda too.



Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:177
 
+static bool needsModules(FrontendInputFile FIF) {
+  switch (FIF.getKind().getLanguage()) {

I think this could be useful for other tools too in the future. Do you think it 
would make sense to put this in a more prominent header, so that other people 
can find it and reuse it more easily?



Comment at: clang/test/ClangScanDeps/deprecated-driver-api.c:1
+// RUN: rm -rf %t
+// RUN: split-file %s %t

Please summarize what this test does.



Comment at: clang/test/ClangScanDeps/multiple-commands.c:1
+// RUN: rm -rf %t
+// RUN: split-file %s %t

Please summarize what this test does.


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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:204
+  /// invocation, for example when there are multiple chained invocations.
+  void handleInvocation(CompilerInvocation CI);
+

jansvoboda11 wrote:
> Nit: `handle*()` functions are associated with `DependencyConsumer` in my 
> brain. Could we find a distinct name to avoid confusion between all the 
> different types we're using here?
I split up applying changes to the CI from passing to the consumer. Hopefully 
it's a little simpler now.



Comment at: clang/test/ClangScanDeps/modules-implicit-dot-private.m:70
 // CHECK:  "-fmodule-file={{.*}}/FW-{{.*}}.pcm"
+// CHECK:  "-fmodule-file={{.*}}/FW_Private-{{.*}}.pcm"
 // CHECK:],

Calling out this change so it doesn't disappear into the other test changes.  
The order changed because in the driver version of this we were appending 
-fmodule-file= options in the order of dependencies, but in the new code we 
insert into `PrebuiltModuleFiles` which is a `std::map`, so the entries are 
alphabetically sorted.


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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 455666.
benlangmuir added a comment.

- Rebased, picking up the changes that were previously split out.
- Broke up applying changes to CI from passing to consumer. The MDC is no 
longer responsible for passing invocations to the consumer, and it is done at 
the Worker level.


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

https://reviews.llvm.org/D132405

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/deprecated-driver-api.c
  clang/test/ClangScanDeps/diagnostics.c
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/ClangScanDeps/modules-context-hash-ignore-macros.c
  clang/test/ClangScanDeps/modules-context-hash-outputs.c
  clang/test/ClangScanDeps/modules-context-hash-warnings.c
  clang/test/ClangScanDeps/modules-context-hash.c
  clang/test/ClangScanDeps/modules-dep-args.c
  clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
  clang/test/ClangScanDeps/modules-full.cpp
  clang/test/ClangScanDeps/modules-implicit-dot-private.m
  clang/test/ClangScanDeps/modules-incomplete-umbrella.c
  clang/test/ClangScanDeps/modules-inferred.m
  clang/test/ClangScanDeps/modules-no-undeclared-includes.c
  clang/test/ClangScanDeps/modules-pch-common-submodule.c
  clang/test/ClangScanDeps/modules-pch-common-via-submodule.c
  clang/test/ClangScanDeps/modules-pch.c
  clang/test/ClangScanDeps/multiple-commands.c
  clang/test/ClangScanDeps/removed-args.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp
  clang/utils/module-deps-to-rsp.py

Index: clang/utils/module-deps-to-rsp.py
===
--- clang/utils/module-deps-to-rsp.py
+++ clang/utils/module-deps-to-rsp.py
@@ -48,6 +48,9 @@
   type=str)
   action.add_argument("--tu-index", help="The index of the translation unit to get arguments for",
   type=int)
+  parser.add_argument("--tu-cmd-index",
+  help="The index of the command within the translation unit (default=0)",
+  type=int, default=0)
   args = parser.parse_args()
 
   full_deps = parseFullDeps(json.load(open(args.full_deps_file, 'r')))
@@ -58,7 +61,8 @@
 if args.module_name:
   cmd = findModule(args.module_name, full_deps)['command-line']
 elif args.tu_index != None:
-  cmd = full_deps.translation_units[args.tu_index]['command-line']
+  tu = full_deps.translation_units[args.tu_index]
+  cmd = tu['commands'][args.tu_cmd_index]['command-line']
 
 print(" ".join(map(quote, cmd)))
   except:
Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -182,6 +182,11 @@
 llvm::cl::desc("The names of dependency targets for the dependency file"),
 llvm::cl::cat(DependencyScannerCategory));
 
+llvm::cl::opt DeprecatedDriverCommand(
+"deprecated-driver-command", llvm::cl::Optional,
+llvm::cl::desc("use a single driver command to build the tu (deprecated)"),
+llvm::cl::cat(DependencyScannerCategory));
+
 enum ResourceDirRecipeKind {
   RDRK_ModifyCompilerPath,
   RDRK_InvokeCompiler,
@@ -256,7 +261,7 @@
 public:
   void mergeDeps(StringRef Input, FullDependenciesResult FDR,
  size_t InputIndex) {
-const FullDependencies  = FDR.FullDeps;
+FullDependencies  = FDR.FullDeps;
 
 InputDeps ID;
 ID.FileName = std::string(Input);
@@ -274,7 +279,8 @@
   Modules.insert(I, {{MD.ID, InputIndex}, std::move(MD)});
 }
 
-ID.CommandLine = FD.CommandLine;
+ID.DriverCommandLine = std::move(FD.DriverCommandLine);
+ID.Commands = std::move(FD.Commands);
 Inputs.push_back(std::move(ID));
   }
 
@@ -311,14 +317,33 @@
 
 Array TUs;
 for (auto & : Inputs) {
-  Object O{
-  {"input-file", I.FileName},
-  {"clang-context-hash", I.ContextHash},
-  {"file-deps", I.FileDeps},
-  {"clang-module-deps", toJSONSorted(I.ModuleDeps)},
-  {"command-line", I.CommandLine},
-  };
-  TUs.push_back(std::move(O));
+  Array Commands;
+  if (I.DriverCommandLine.empty()) {
+for (const auto  : I.Commands) {
+  Object O{
+  {"input-file", I.FileName},
+  {"clang-context-hash", I.ContextHash},
+  {"file-deps", I.FileDeps},
+  {"clang-module-deps", toJSONSorted(I.ModuleDeps)},
+  {"executable", Cmd.Executable},
+  {"command-line", 

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:224
+  /// a preprocessor. Storage owned by \c ModularDeps.
+  llvm::StringMap ModuleDepsByID;
+  /// Directly imported prebuilt deps.

jansvoboda11 wrote:
> I assume you're not using `llvm::DenseMap` because 
> it's a hassle to implement for custom keys and performance is not a concern, 
> correct? Any other aspects?
No good reason.  I switched it to DenseMap and included the change in 
https://reviews.llvm.org/D132617



Comment at: 
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:226
+  /// Directly imported prebuilt deps.
+  std::vector DirectPrebuiltDeps;
+

jansvoboda11 wrote:
> Would this allow us to remove 
> `ModuleDepCollectorPP::DirectPrebuiltModularDeps`?
I sunk it to MDC in https://reviews.llvm.org/D132617.  It needed to change to 
`MapVector` since we are also uniquing them.



Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:358
+  for (auto & : DirectPrebuiltModularDeps) {
+MDC.DirectPrebuiltDeps.emplace_back(I);
+MDC.Consumer.handlePrebuiltModuleDependency(MDC.DirectPrebuiltDeps.back());

jansvoboda11 wrote:
> As said in a previous comment, we might avoid the copy by storing this in 
> `MDC` in the first place.
We don't save any copies, since we need to actually store the 
`PrebuiltModuleDep` in `MDC` in order to apply it to a secondary 
CompilerInvocation after the preprocessor etc. is gone.  But I still like 
sinking it down.



Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:471
+
+  MDC.setModuleDepsForID(MD.ID, );
   return MD.ID;

jansvoboda11 wrote:
> Nit: this function might get easier to think about if we did this in one step 
> with the context hash calculation:
> 
> ```
> MDC.associateWithContextHash(MD, CI);
> MDC.addOutputPaths(MD, CI);
> MD.BuildArguments = CI.getCC1CommandLine();
> ```
Good idea, included in https://reviews.llvm.org/D132617


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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

In D132405#3747232 , @jansvoboda11 
wrote:

> I'd like to see this split into multiple patches. I can see some formatting 
> changes, removal of `CompilerInvocation` from `ModuleDeps`, isolated changes 
> to `Tooling`, etc. That'd make it much easier to review.

`ToolInvocation` change: https://reviews.llvm.org/D132615
Remove `CompilerInvocation` from `ModuleDeps`: https://reviews.llvm.org/D132616
Factoring out the `addModule*Files` functions, sink 
`DirectPrebuiltModularDeps`, etc: https://reviews.llvm.org/D132617


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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

I left a couple of smaller initial comments. I'd like to see this split into 
multiple patches. I can see some formatting changes, removal of 
`CompilerInvocation` from `ModuleDeps`, isolated changes to `Tooling`, etc. 
That'd make it much easier to review.




Comment at: 
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:204
+  /// invocation, for example when there are multiple chained invocations.
+  void handleInvocation(CompilerInvocation CI);
+

Nit: `handle*()` functions are associated with `DependencyConsumer` in my 
brain. Could we find a distinct name to avoid confusion between all the 
different types we're using here?



Comment at: 
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:224
+  /// a preprocessor. Storage owned by \c ModularDeps.
+  llvm::StringMap ModuleDepsByID;
+  /// Directly imported prebuilt deps.

I assume you're not using `llvm::DenseMap` because it's 
a hassle to implement for custom keys and performance is not a concern, 
correct? Any other aspects?



Comment at: 
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h:226
+  /// Directly imported prebuilt deps.
+  std::vector DirectPrebuiltDeps;
+

Would this allow us to remove `ModuleDepCollectorPP::DirectPrebuiltModularDeps`?



Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:358
+  for (auto & : DirectPrebuiltModularDeps) {
+MDC.DirectPrebuiltDeps.emplace_back(I);
+MDC.Consumer.handlePrebuiltModuleDependency(MDC.DirectPrebuiltDeps.back());

As said in a previous comment, we might avoid the copy by storing this in `MDC` 
in the first place.



Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:471
+
+  MDC.setModuleDepsForID(MD.ID, );
   return MD.ID;

Nit: this function might get easier to think about if we did this in one step 
with the context hash calculation:

```
MDC.associateWithContextHash(MD, CI);
MDC.addOutputPaths(MD, CI);
MD.BuildArguments = CI.getCC1CommandLine();
```


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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 455338.

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

https://reviews.llvm.org/D132405

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/include/clang/Tooling/Tooling.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/test/ClangScanDeps/deprecated-driver-api.c
  clang/test/ClangScanDeps/diagnostics.c
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/ClangScanDeps/modules-context-hash-ignore-macros.c
  clang/test/ClangScanDeps/modules-context-hash-outputs.c
  clang/test/ClangScanDeps/modules-context-hash-warnings.c
  clang/test/ClangScanDeps/modules-context-hash.c
  clang/test/ClangScanDeps/modules-dep-args.c
  clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
  clang/test/ClangScanDeps/modules-full.cpp
  clang/test/ClangScanDeps/modules-inferred.m
  clang/test/ClangScanDeps/modules-no-undeclared-includes.c
  clang/test/ClangScanDeps/modules-pch-common-submodule.c
  clang/test/ClangScanDeps/modules-pch-common-via-submodule.c
  clang/test/ClangScanDeps/modules-pch.c
  clang/test/ClangScanDeps/multiple-commands.c
  clang/test/ClangScanDeps/removed-args.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp
  clang/unittests/Tooling/ToolingTest.cpp
  clang/utils/module-deps-to-rsp.py

Index: clang/utils/module-deps-to-rsp.py
===
--- clang/utils/module-deps-to-rsp.py
+++ clang/utils/module-deps-to-rsp.py
@@ -48,6 +48,9 @@
   type=str)
   action.add_argument("--tu-index", help="The index of the translation unit to get arguments for",
   type=int)
+  parser.add_argument("--tu-cmd-index",
+  help="The index of the command within the translation unit (default=0)",
+  type=int, default=0)
   args = parser.parse_args()
 
   full_deps = parseFullDeps(json.load(open(args.full_deps_file, 'r')))
@@ -58,7 +61,8 @@
 if args.module_name:
   cmd = findModule(args.module_name, full_deps)['command-line']
 elif args.tu_index != None:
-  cmd = full_deps.translation_units[args.tu_index]['command-line']
+  tu = full_deps.translation_units[args.tu_index]
+  cmd = tu['commands'][args.tu_cmd_index]['command-line']
 
 print(" ".join(map(quote, cmd)))
   except:
Index: clang/unittests/Tooling/ToolingTest.cpp
===
--- clang/unittests/Tooling/ToolingTest.cpp
+++ clang/unittests/Tooling/ToolingTest.cpp
@@ -326,6 +326,46 @@
   EXPECT_TRUE(Consumer.SawSourceManager);
 }
 
+TEST(ToolInvocation, CC1Args) {
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new llvm::vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+  llvm::IntrusiveRefCntPtr Files(
+  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  std::vector Args;
+  Args.push_back("tool-executable");
+  Args.push_back("-cc1");
+  Args.push_back("-fsyntax-only");
+  Args.push_back("test.cpp");
+  clang::tooling::ToolInvocation Invocation(
+  Args, std::make_unique(), Files.get());
+  InMemoryFileSystem->addFile(
+  "test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("void foo(void);\n"));
+  EXPECT_TRUE(Invocation.run());
+}
+
+TEST(ToolInvocation, CC1ArgsInvalid) {
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new llvm::vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+  llvm::IntrusiveRefCntPtr Files(
+  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  std::vector Args;
+  Args.push_back("tool-executable");
+  Args.push_back("-cc1");
+  Args.push_back("-invalid-arg");
+  Args.push_back("test.cpp");
+  clang::tooling::ToolInvocation Invocation(
+  Args, std::make_unique(), Files.get());
+  InMemoryFileSystem->addFile(
+  "test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("void foo(void);\n"));
+  EXPECT_FALSE(Invocation.run());
+}
+
 namespace {
 /// Overlays the real filesystem with the given VFS and returns the result.
 llvm::IntrusiveRefCntPtr
Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -182,6 +182,11 @@
 llvm::cl::desc("The names of dependency targets for 

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir marked 11 inline comments as done.
benlangmuir added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:44
+
+  enum CommandKind {
+CK_CC1,

tschuett wrote:
> Why is this not an enum class?
Removed in latest change.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:80
+
+  static bool classof(const Command *C) { return C->getKind() == CK_Simple; }
+};

tschuett wrote:
> Why are all members public?
Latest change simplifies this to a struct.



Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:155
+void FullDependencyConsumer::handleInvocation(CompilerInvocation CI) {
+  clearImplicitModuleBuildOptions(CI);
+  Commands.push_back(std::make_unique(std::move(CI)));

jansvoboda11 wrote:
> benlangmuir wrote:
> > jansvoboda11 wrote:
> > > Shouldn't this be a responsibility of the dependency scanner?
> > Good question! I was mirroring how it works for modules where we do this in 
> > the `ModuleDepCollector` and store the whole invocation in the 
> > `ModuleDeps`.  But I guess that's "inside" the dep scanner, and this is 
> > "outside".  Happy to shuffle things around.
> > 
> > What is your opinion of  `takeFullDependencies()` adding to 
> > `FrontendOpts.ModuleFiles`, ? That is also arguably the scanner's 
> > responsibility.
> > 
> > Another thing we could do here is remove `BuildInvocation` from 
> > `FullDependencies` and `ModuleDeps` and only expose the serialized `-cc1` 
> > arguments.  It would simplify the new code a bit since there'd only be one 
> > kind of "command".   On the other hand, I could also see it being 
> > potentially useful to have the whole invocation available if you were 
> > writing a C++ tool that used the scanner API rather than just 
> > clang-scan-deps itself.
> > Good question! I was mirroring how it works for modules where we do this in 
> > the `ModuleDepCollector` and store the whole invocation in the 
> > `ModuleDeps`. But I guess that's "inside" the dep scanner, and this is 
> > "outside". Happy to shuffle things around.
> As discussed offline, I'd prefer keeping `CompilerInvocation` internal to the 
> dependency scanner and exposing simple types (`std::vector` for 
> the command line) unless clients need more flexibility.
> 
> > What is your opinion of `takeFullDependencies()` adding to 
> > `FrontendOpts.ModuleFiles`, ? That is also arguably the scanner's 
> > responsibility.
> I'd prefer adding to `FrontendOpts.ModuleFiles` earlier, before the consumer.
> 
> > Another thing we could do here is remove `BuildInvocation` from 
> > `FullDependencies` and `ModuleDeps` and only expose the serialized `-cc1` 
> > arguments. It would simplify the new code a bit since there'd only be one 
> > kind of "command". On the other hand, I could also see it being potentially 
> > useful to have the whole invocation available if you were writing a C++ 
> > tool that used the scanner API rather than just clang-scan-deps itself.
> +1
Done in latest changes. The logic for mutating the compiler invocation is now 
contained in the ModuleDepCollector, alongside the existing logic for doing the 
same kind of changes to module build commands.


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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir updated this revision to Diff 455315.
benlangmuir added a comment.

- Remove CompilerInvocation from Command and ModuleDeps. Only the arg strings 
are exposed now.
- Simplify Command, which is now just a simple struct.
- Move the logic for mutating the CompilerInvocation for the TU into the 
scanner.
- Minor refactoring to reduce nesting in DependencyScanningWorker.
- Rebase


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

https://reviews.llvm.org/D132405

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
  clang/include/clang/Tooling/Tooling.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/test/ClangScanDeps/deprecated-driver-api.c
  clang/test/ClangScanDeps/diagnostics.c
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/ClangScanDeps/modules-context-hash-ignore-macros.c
  clang/test/ClangScanDeps/modules-context-hash-outputs.c
  clang/test/ClangScanDeps/modules-context-hash-warnings.c
  clang/test/ClangScanDeps/modules-context-hash.c
  clang/test/ClangScanDeps/modules-dep-args.c
  clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
  clang/test/ClangScanDeps/modules-full.cpp
  clang/test/ClangScanDeps/modules-inferred.m
  clang/test/ClangScanDeps/modules-no-undeclared-includes.c
  clang/test/ClangScanDeps/modules-pch-common-submodule.c
  clang/test/ClangScanDeps/modules-pch-common-via-submodule.c
  clang/test/ClangScanDeps/modules-pch.c
  clang/test/ClangScanDeps/multiple-commands.c
  clang/test/ClangScanDeps/removed-args.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp
  clang/unittests/Tooling/ToolingTest.cpp
  clang/utils/module-deps-to-rsp.py

Index: clang/utils/module-deps-to-rsp.py
===
--- clang/utils/module-deps-to-rsp.py
+++ clang/utils/module-deps-to-rsp.py
@@ -48,6 +48,9 @@
   type=str)
   action.add_argument("--tu-index", help="The index of the translation unit to get arguments for",
   type=int)
+  parser.add_argument("--tu-cmd-index",
+  help="The index of the command within the translation unit (default=0)",
+  type=int, default=0)
   args = parser.parse_args()
 
   full_deps = parseFullDeps(json.load(open(args.full_deps_file, 'r')))
@@ -58,7 +61,8 @@
 if args.module_name:
   cmd = findModule(args.module_name, full_deps)['command-line']
 elif args.tu_index != None:
-  cmd = full_deps.translation_units[args.tu_index]['command-line']
+  tu = full_deps.translation_units[args.tu_index]
+  cmd = tu['commands'][args.tu_cmd_index]['command-line']
 
 print(" ".join(map(quote, cmd)))
   except:
Index: clang/unittests/Tooling/ToolingTest.cpp
===
--- clang/unittests/Tooling/ToolingTest.cpp
+++ clang/unittests/Tooling/ToolingTest.cpp
@@ -326,6 +326,46 @@
   EXPECT_TRUE(Consumer.SawSourceManager);
 }
 
+TEST(ToolInvocation, CC1Args) {
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new llvm::vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+  llvm::IntrusiveRefCntPtr Files(
+  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  std::vector Args;
+  Args.push_back("tool-executable");
+  Args.push_back("-cc1");
+  Args.push_back("-fsyntax-only");
+  Args.push_back("test.cpp");
+  clang::tooling::ToolInvocation Invocation(
+  Args, std::make_unique(), Files.get());
+  InMemoryFileSystem->addFile(
+  "test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("void foo(void);\n"));
+  EXPECT_TRUE(Invocation.run());
+}
+
+TEST(ToolInvocation, CC1ArgsInvalid) {
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new llvm::vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+  llvm::IntrusiveRefCntPtr Files(
+  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  std::vector Args;
+  Args.push_back("tool-executable");
+  Args.push_back("-cc1");
+  Args.push_back("-invalid-arg");
+  Args.push_back("test.cpp");
+  clang::tooling::ToolInvocation Invocation(
+  Args, std::make_unique(), Files.get());
+  InMemoryFileSystem->addFile(
+  "test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("void foo(void);\n"));
+  EXPECT_FALSE(Invocation.run());
+}
+
 namespace {
 /// Overlays the real filesystem with the given VFS and 

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:34
+/// \see FullDependencies::Commands.
+class Command {
+public:

benlangmuir wrote:
> jansvoboda11 wrote:
> > Have you considered using the `Job`/`Command` classes the driver uses? What 
> > are the downsides of doing that?
> The `driver::Command`/`driver::JobList` is not standalone from the driver - 
> it depends on an external `const Action &`, `const Tool &`, and a few `const 
> char *`, so if we used it we would also need to preserve the `Compilation` 
> and `Driver` they came from somewhere, or change  the API so the commands 
> were only available inside a callback with limited scope that they are 
> immediately copied out of.  I'm also not sure what the consequences of 
> mutating Commands are (there is a `replaceArguments`, but I don't know if it 
> could break invariants).
Makes sense, let's keep this as-is then.



Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:155
+void FullDependencyConsumer::handleInvocation(CompilerInvocation CI) {
+  clearImplicitModuleBuildOptions(CI);
+  Commands.push_back(std::make_unique(std::move(CI)));

benlangmuir wrote:
> jansvoboda11 wrote:
> > Shouldn't this be a responsibility of the dependency scanner?
> Good question! I was mirroring how it works for modules where we do this in 
> the `ModuleDepCollector` and store the whole invocation in the `ModuleDeps`.  
> But I guess that's "inside" the dep scanner, and this is "outside".  Happy to 
> shuffle things around.
> 
> What is your opinion of  `takeFullDependencies()` adding to 
> `FrontendOpts.ModuleFiles`, ? That is also arguably the scanner's 
> responsibility.
> 
> Another thing we could do here is remove `BuildInvocation` from 
> `FullDependencies` and `ModuleDeps` and only expose the serialized `-cc1` 
> arguments.  It would simplify the new code a bit since there'd only be one 
> kind of "command".   On the other hand, I could also see it being potentially 
> useful to have the whole invocation available if you were writing a C++ tool 
> that used the scanner API rather than just clang-scan-deps itself.
> Good question! I was mirroring how it works for modules where we do this in 
> the `ModuleDepCollector` and store the whole invocation in the `ModuleDeps`. 
> But I guess that's "inside" the dep scanner, and this is "outside". Happy to 
> shuffle things around.
As discussed offline, I'd prefer keeping `CompilerInvocation` internal to the 
dependency scanner and exposing simple types (`std::vector` for 
the command line) unless clients need more flexibility.

> What is your opinion of `takeFullDependencies()` adding to 
> `FrontendOpts.ModuleFiles`, ? That is also arguably the scanner's 
> responsibility.
I'd prefer adding to `FrontendOpts.ModuleFiles` earlier, before the consumer.

> Another thing we could do here is remove `BuildInvocation` from 
> `FullDependencies` and `ModuleDeps` and only expose the serialized `-cc1` 
> arguments. It would simplify the new code a bit since there'd only be one 
> kind of "command". On the other hand, I could also see it being potentially 
> useful to have the whole invocation available if you were writing a C++ tool 
> that used the scanner API rather than just clang-scan-deps itself.
+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h:102
+std::vector
+serializeCompilerInvocation(const CompilerInvocation );
+

jansvoboda11 wrote:
> Maybe we could add this API directly to `CompilerInvocation`. I can see lots 
> of clients not caring about the (potentially more efficient) 
> `CompilerInvocation::generateCC1CommandLine()` with `StringAllocator` and 
> inventing their own implementation of this.
https://reviews.llvm.org/D132419



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:438
+
+void dependencies::clearImplicitModuleBuildOptions(CompilerInvocation ) {
+  CI.getLangOpts()->ImplicitModules = false;

jansvoboda11 wrote:
> Wondering if we could move this to `CompilerInvocation`, right besides 
> `resetNonModularOptions()`.
https://reviews.llvm.org/D132419


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:34
+/// \see FullDependencies::Commands.
+class Command {
+public:

jansvoboda11 wrote:
> Have you considered using the `Job`/`Command` classes the driver uses? What 
> are the downsides of doing that?
The `driver::Command`/`driver::JobList` is not standalone from the driver - it 
depends on an external `const Action &`, `const Tool &`, and a few `const char 
*`, so if we used it we would also need to preserve the `Compilation` and 
`Driver` they came from somewhere, or change  the API so the commands were only 
available inside a callback with limited scope that they are immediately copied 
out of.  I'm also not sure what the consequences of mutating Commands are 
(there is a `replaceArguments`, but I don't know if it could break invariants).



Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:155
+void FullDependencyConsumer::handleInvocation(CompilerInvocation CI) {
+  clearImplicitModuleBuildOptions(CI);
+  Commands.push_back(std::make_unique(std::move(CI)));

jansvoboda11 wrote:
> Shouldn't this be a responsibility of the dependency scanner?
Good question! I was mirroring how it works for modules where we do this in the 
`ModuleDepCollector` and store the whole invocation in the `ModuleDeps`.  But I 
guess that's "inside" the dep scanner, and this is "outside".  Happy to shuffle 
things around.

What is your opinion of  `takeFullDependencies()` adding to 
`FrontendOpts.ModuleFiles`, ? That is also arguably the scanner's 
responsibility.

Another thing we could do here is remove `BuildInvocation` from 
`FullDependencies` and `ModuleDeps` and only expose the serialized `-cc1` 
arguments.  It would simplify the new code a bit since there'd only be one kind 
of "command".   On the other hand, I could also see it being potentially useful 
to have the whole invocation available if you were writing a C++ tool that used 
the scanner API rather than just clang-scan-deps itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:34
+/// \see FullDependencies::Commands.
+class Command {
+public:

Have you considered using the `Job`/`Command` classes the driver uses? What are 
the downsides of doing that?



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h:37
+ std::vector Args) = 0;
+  virtual void handleInvocation(clang::CompilerInvocation CI) = 0;
+

Is `clang::` necessary here?



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h:102
+std::vector
+serializeCompilerInvocation(const CompilerInvocation );
+

Maybe we could add this API directly to `CompilerInvocation`. I can see lots of 
clients not caring about the (potentially more efficient) 
`CompilerInvocation::generateCC1CommandLine()` with `StringAllocator` and 
inventing their own implementation of this.



Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:155
+void FullDependencyConsumer::handleInvocation(CompilerInvocation CI) {
+  clearImplicitModuleBuildOptions(CI);
+  Commands.push_back(std::make_unique(std::move(CI)));

Shouldn't this be a responsibility of the dependency scanner?



Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:197
+for (const auto  : FD.ClangModuleDeps)
+  FrontendOpts.ModuleFiles.push_back(
+  LookupModuleOutput(ID, ModuleOutputKind::ModuleFile));

Just a heads-up: after I land D132066 today, you might need to update this to 
respect the eager/lazy loading mode.



Comment at: 
clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:438
+
+void dependencies::clearImplicitModuleBuildOptions(CompilerInvocation ) {
+  CI.getLangOpts()->ImplicitModules = false;

Wondering if we could move this to `CompilerInvocation`, right besides 
`resetNonModularOptions()`.



Comment at: clang/test/ClangScanDeps/diagnostics.c:31
 // CHECK-NEXT: {
-// CHECK-NEXT:   "clang-context-hash": "[[HASH_TU:.*]],
+// CHECK:"clang-context-hash": "[[HASH_TU:.*]],
 // CHECK-NEXT:   "clang-module-deps": [

benlangmuir wrote:
> I didn't want to radically change all the tests, so in most cases I just 
> loosened the matching so it would match inside the "commands".  The 
> modules-full.c and multiple-commands.c cover the whole structure in detail.
That sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Thorsten via Phabricator via cfe-commits]

tschuett added inline comments.



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:44
+
+  enum CommandKind {
+CK_CC1,

Why is this not an enum class?



Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:80
+
+  static bool classof(const Command *C) { return C->getKind() == CK_Simple; }
+};

Why are all members public?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.
Herald added a subscriber: ormris.



Comment at: clang/test/ClangScanDeps/diagnostics.c:31
 // CHECK-NEXT: {
-// CHECK-NEXT:   "clang-context-hash": "[[HASH_TU:.*]],
+// CHECK:"clang-context-hash": "[[HASH_TU:.*]],
 // CHECK-NEXT:   "clang-module-deps": [

I didn't want to radically change all the tests, so in most cases I just 
loosened the matching so it would match inside the "commands".  The 
modules-full.c and multiple-commands.c cover the whole structure in detail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132405

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

[PATCH] D132405: [clang][deps] Split translation units into individual -cc1 or other commands

[Ben Langmuir via Phabricator via cfe-commits]

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

Instead of trying to "fix" the original driver invocation by appending 
arguments to it, split it into multiple commands, and for each -cc1 command use 
a CompilerInvocation to give precise control over the invocation.

This change should make it easier to (in the future) canonicalize the 
command-line (e.g. to improve hits in something like ccache), apply 
optimizations, or start supporting multi-arch builds, which would require 
different modules for each arch.

In the long run it may make sense to treat the TU commands as a dependency 
graph, each with their own dependencies on modules or earlier TU commands, but 
for now they are simply a list that is executed in order, and the dependencies 
are simply duplicated. Since we currently only support single-arch builds, 
there is no parallelism available in the execution.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132405

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h
  clang/include/clang/Tooling/Tooling.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/lib/Tooling/Tooling.cpp
  clang/test/ClangScanDeps/deprecated-driver-api.c
  clang/test/ClangScanDeps/diagnostics.c
  clang/test/ClangScanDeps/header-search-pruning-transitive.c
  clang/test/ClangScanDeps/modules-context-hash-ignore-macros.c
  clang/test/ClangScanDeps/modules-context-hash-outputs.c
  clang/test/ClangScanDeps/modules-context-hash-warnings.c
  clang/test/ClangScanDeps/modules-context-hash.c
  clang/test/ClangScanDeps/modules-fmodule-name-no-module-built.m
  clang/test/ClangScanDeps/modules-full.cpp
  clang/test/ClangScanDeps/modules-inferred.m
  clang/test/ClangScanDeps/modules-no-undeclared-includes.c
  clang/test/ClangScanDeps/modules-pch-common-submodule.c
  clang/test/ClangScanDeps/modules-pch-common-via-submodule.c
  clang/test/ClangScanDeps/modules-pch.c
  clang/test/ClangScanDeps/multiple-commands.c
  clang/test/ClangScanDeps/removed-args.c
  clang/tools/clang-scan-deps/ClangScanDeps.cpp
  clang/unittests/Tooling/ToolingTest.cpp
  clang/utils/module-deps-to-rsp.py

Index: clang/utils/module-deps-to-rsp.py
===
--- clang/utils/module-deps-to-rsp.py
+++ clang/utils/module-deps-to-rsp.py
@@ -48,6 +48,9 @@
   type=str)
   action.add_argument("--tu-index", help="The index of the translation unit to get arguments for",
   type=int)
+  parser.add_argument("--tu-cmd-index",
+  help="The index of the command within the translation unit (default=0)",
+  type=int, default=0)
   args = parser.parse_args()
 
   full_deps = parseFullDeps(json.load(open(args.full_deps_file, 'r')))
@@ -58,7 +61,8 @@
 if args.module_name:
   cmd = findModule(args.module_name, full_deps)['command-line']
 elif args.tu_index != None:
-  cmd = full_deps.translation_units[args.tu_index]['command-line']
+  tu = full_deps.translation_units[args.tu_index]
+  cmd = tu['commands'][args.tu_cmd_index]['command-line']
 
 print(" ".join(map(quote, cmd)))
   except:
Index: clang/unittests/Tooling/ToolingTest.cpp
===
--- clang/unittests/Tooling/ToolingTest.cpp
+++ clang/unittests/Tooling/ToolingTest.cpp
@@ -326,6 +326,46 @@
   EXPECT_TRUE(Consumer.SawSourceManager);
 }
 
+TEST(ToolInvocation, CC1Args) {
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new llvm::vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+  llvm::IntrusiveRefCntPtr Files(
+  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  std::vector Args;
+  Args.push_back("tool-executable");
+  Args.push_back("-cc1");
+  Args.push_back("-fsyntax-only");
+  Args.push_back("test.cpp");
+  clang::tooling::ToolInvocation Invocation(
+  Args, std::make_unique(), Files.get());
+  InMemoryFileSystem->addFile(
+  "test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("void foo(void);\n"));
+  EXPECT_TRUE(Invocation.run());
+}
+
+TEST(ToolInvocation, CC1ArgsInvalid) {
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new llvm::vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+  llvm::IntrusiveRefCntPtr Files(