[PATCH] D133161: [Clang] Fix the new driver crashing when using '-fsyntax-only'

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:4396-4398
+  bool SingleDeviceOutput = !llvm::any_of(OffloadActions, [](Action *A) {
+    return A->getType() == types::TY_Nothing;
+  }) && isa<CompileJobAction>(HostAction);
----------------
tra wrote:
> `any_of(A->getType() == types::TY_Nothing)` looks like a rather indirect way to infer that we have `-fsyntax-only`. Would there be any other legitimate case when we'd have `Ty_Nothing` actions?
> 
> 
> 
This is more or less how the existing pipeline handles it. We shouldn't propagate `TY_Nothing` attributes regardless, `-fsyntax-only` just happens to create them. So this is more correct I believe.


================
Comment at: clang/lib/Driver/Driver.cpp:4402
       /*BoundArch=*/nullptr, isa<CompileJobAction>(HostAction) ? DDep : DDeps);
-  return C.MakeAction<OffloadAction>(
-      HDep, isa<CompileJobAction>(HostAction) ? DDep : DDeps);
+  return C.MakeAction<OffloadAction>(HDep, SingleDeviceOutput ? DDep : DDeps);
 }
----------------
tra wrote:
> Assuming that we're guaranteed that all actions produce no outputs, can we just pass an empty array if `-fsyntax-only` is specified?
> 
> 
No, this is required to make the device actions be generated. The old driver manually added device actions to the final action list, while the new driver adds them as host dependencies. I prefer the new method as it requires no internal state so it can just be a simple function like here.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4503
   for (const InputInfo &I : Inputs) {
-    if (&I == &Input) {
-      // This is the primary input.
+    if (&I == &Input || I.getType() == types::TY_Nothing) {
+      // This is the primary input or contains nothing.
----------------
tra wrote:
> .. and if we don't propagate no-output actions, there would be no need to filter them here.
Without that we would need to change the logic to append to the final action list directly, which would be an enormous hack given the current architecture of the new driver. FWIW I think this is a valid operation regardless, this didn't have a good failure mode in the first place, it just crashed on some assertion later if you did it. This at least makes sure that we'll get some "no input" error message AFAIK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133161