Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[jlebar]

Summary

When compiling CUDA, we run the frontend N times, once for each device
arch. This means that if you have a compile error in your file, you'll
see that error N times.

Relatedly, if ptxas fails, we'll output that error and then still try to
pass its output to fatbinary, which then fails because (duh) its input
file doesn't exist.

This patch stops compilations with -stop-on-failure as soon as we
encounter an error. -stop-on-failure is turned on by default for CUDA
compilations.

[jlebar]

Friendly ping.

[tra]

include/clang/Driver/Options.td

1807

I'd use 'no-' prefix.

lib/Driver/Driver.cpp

650

Why is StopOnFailure is false in this case? Shouldn't it obey command line options, too?

[jlebar]

Address tra's review comment (rename flag).

lib/Driver/Driver.cpp

650

This function is called when the compiler has an internal error or crashes. The jobs we're executing here are preprocessor jobs dumping debugging info. I figured we should not stop on failure when outputting that info?

[echristo]

In general it feels like keeping 2 errors might make the most sense:

(Using a multiarch build rather than a cuda command line, but it should still be the same behavior for consistency)

t.c:

#if _NOT_ARCH4_
#error "aiee!"
#endif

clang -arch arch1 -arch arch2 -arch arch3 -arch arch4 t.c

seems like it might be nice to get 3 errors here rather than a single one and fixing that single one, then getting another one, etc. or realizing what the error is here.

I don't feel strongly about this, but I'm still uncertain as to why we want to make things more complicated here :)

-eric

[tra]

LGTM.

lib/Driver/Driver.cpp

650

As far as I can tell, we don't do anything interesting if we've detected that *any* of the commands have failed. That suggests that doing anything beyond the first failing command does not do us any good. That would suggest that we may really want StopOnFailure=true here.

'false' would preserve current behavior, though.

In either case I'm OK with a constant here.

[jlebar]

In D16514#338631, @echristo wrote:

In general it feels like keeping 2 errors might make the most sense:

#if _NOT_ARCH4_
#error "aiee!"
#endif

clang -arch arch1 -arch arch2 -arch arch3 -arch arch4 t.c

seems like it might be nice to get 3 errors here rather than a single one and fixing that single one, then getting another one, etc. or realizing what the error is here.

Yes, this patch makes that case worse.

But I suspect errors that apply to some but not all archs will be far less common than errors that apply to all arches -- regular C++ errors like missing a semicolon or whatever. It feels pretty overwhelming to output N copies of every error in those cases, especially when you consider multipage template errors.

In addition, iirc there's no separation between errors outputted for different archs, so it really looks like we're just outputting multiple copies of the errors for fun.

I don't feel strongly about this, but I'm still uncertain as to why we want to make things more complicated here :)

The other reason, which is less important, is that when you have one arch and ptxas fails -- which, it shouldn't, but we're not good enough to catch everything yet, and likely won't be for some time -- the error you get is

ptxas: foo is not defined
*FATAL ERROR*: fatbinary failed, /tmp/bar.cubin does not exist.

I'd like not to display that second line, since it hides the actual problem. Once you get used to it, it's not a big deal, but it tripped me up for a few minutes, and I'm the one who added the call to ptxas.

lib/Driver/Driver.cpp

650

Sorry, I think I'm misunderstanding something. Would you mind rephrasing this?

As far as I can tell, we don't do anything interesting if we've detected that *any* of the commands have failed. That suggests that doing anything beyond the first failing command does not do us any good.

The scenario I thought this change applied to was:

External tool crashes during a call to ExecuteJobs() (not this one). We now want to output preprocessed inputs, so we run this code, which again calls ExecuteJobs(), but these jobs only run the preprocessor on the inputs.

Now suppose one of those preprocessor jobs fails. Maybe it has a bad preprocessor directive, or maybe #error would be enough. It seems to me in this case that we should continue running the other preprocessor jobs, so we dump as much debug info as we can.

Note that if the StopOnFailure flag is false, afaict it's entirely possible for us to have two inputs, one of which has a pp error and the other of which causes a compiler crash -- if we stopped on failure here, we wouldn't output anything for the second input, which is the one we're interested in.

Sorry again, I'm sure I'm missing something.

[tra]

lib/Driver/Driver.cpp

650

Look at the lines below. If there are any failing commands we just report an error and return.
Even if there are multiple preprocessor jobs and if some of them succeed, we would not get to use their output.

[jlebar]

Pass StopOnFailure = true when running the preprocessor after an ICE.

[jlebar]

lib/Driver/Driver.cpp

650

Oh.

Thanks. :)

[echristo]

The other reason, which is less important, is that when you have one arch and ptxas fails -- which, it shouldn't, but we're not good enough to catch everything yet, and likely won't be for some time -- the error you get is

ptxas: foo is not defined
*FATAL ERROR*: fatbinary failed, /tmp/bar.cubin does not exist.

I'd like not to display that second line, since it hides the actual problem. Once you get used to it, it's not a big deal, but it tripped me up for a few minutes, and I'm the one who added the call to ptxas.

This seems like more of a problem - we don't have this with the existing BindArchAction set of things, we stop before trying to call lipo on darwin. Hrm.

-eric

[jlebar]

Eric, are you OK with this going in, or do you want to consider alternatives?

[jlebar]

Talking to echristo irl, he would like to know why we don't have this problem with mac universal binaries -- or, do we? He would like to be consistent; I'm onboard with that.

[jlebar]

Okay, I see why things don't work as expected without this patch but do work for e.g. macos universal binaries.

The reason is, we build a completely separate set of actions for each invocation of cc1 -- one for the host compilation, and one for each device arch. Then the logic inside Compilation.cpp, which is in fact trying not to display duplicate errors, doesn't work, because it doesn't know that these compilations are related.

I think I may be able to fix this.

[jlebar]

Per IRL discussion with echristo, updated so that we just bail as soon as one
subjob fails.

[echristo]

This works for me and I can't think of anything it's going to break so LGTM.

Thanks!

-eric

[jlebar]

espindola reverted this in r260522 because of test failures in Driver/output-file-cleanup.c.

The reason I didn't catch this locally is that the test is non-hermetic -- if it passed once in an objdir, this patch does not make it fail again. You have to nuke (part of) the objdir before it will fail.

I'll send a patch to make the test hermetic. Unsure yet whether it's a bug in this patch or the test that the test fails at all.

include/clang/Driver/Compilation.h

public:
 
   /// ExecuteCommand - Execute an actual command.
   ///
   /// \param FailingCommand - For non-zero results, this will be set to the
   /// Command which failed, if any.
   /// \return The result code of the subprocess.
   int ExecuteCommand(const Command &C, const Command *&FailingCommand) const;
 
-  /// ExecuteJob - Execute a single job.
+  /// ExecuteJobs - Execute a list of jobs.
   ///
-  /// \param FailingCommands - For non-zero results, this will be a vector of
-  /// failing commands and their associated result code.
+  /// \param StopOnFailure - If true, execution stops as soon as one job fails.
+  /// \param FailingCommands - Outparam that's set to the list of Commands that
+  /// failed, plus their associated result codes.
   void ExecuteJobs(
-      const JobList &Jobs,
+      const JobList &Jobs, bool StopOnFailure,
       SmallVectorImpl<std::pair<int, const Command *>> &FailingCommands) const;
 
   /// initCompilationForDiagnostics - Remove stale state and suppress output
   /// so compilation can be reexecuted to generate additional diagnostic
   /// information (e.g., preprocessed source(s)).
   void initCompilationForDiagnostics();
 
   /// Return true if we're compiling for diagnostics.
   bool isForDiagnostics() const { return ForDiagnostics; }
 };
 
 } // end namespace driver
 } // end namespace clang
 
 #endif

include/clang/Driver/Driver.h

 public:
   /// Use lazy precompiled headers for PCH support.
   unsigned CCCUsePCH : 1;
 
 private:
   /// Certain options suppress the 'no input files' warning.
   bool SuppressMissingInputWarning : 1;
 
+  /// Should we stop running all jobs as soon as one fails?  If false, we run as
+  /// much as we can.
+  bool StopOnJobFailure : 1;
+
   std::list<std::string> TempFiles;
   std::list<std::string> ResultFiles;
 
   /// \brief Cache of all the ToolChains in use by the driver.
   ///
   /// This maps from the string representation of a triple to a ToolChain
   /// created targeting that triple. The driver owns all the ToolChain objects
   /// stored in it, and will clean them up when torn down.

include/clang/Driver/Options.td

def fintegrated_as : Flag<["-"], "fintegrated-as">, Flags<[DriverOption]>,
                      Group<f_Group>, HelpText<"Enable the integrated assembler">;
 def fno_integrated_as : Flag<["-"], "fno-integrated-as">,
                         Flags<[CC1Option, DriverOption]>, Group<f_Group>,
                         HelpText<"Disable the integrated assembler">;
 def : Flag<["-"], "integrated-as">, Alias<fintegrated_as>, Flags<[DriverOption]>;
 def : Flag<["-"], "no-integrated-as">, Alias<fno_integrated_as>,
       Flags<[CC1Option, DriverOption]>;
 
+def stop_on_failure : Flag<["-"], "stop-on-failure">, Flags<[DriverOption]>,
+  HelpText<"Stop running jobs as soon as one fails.  This is the default during "
+    "CUDA compilation without --save-temps.">;
+def nostop_on_failure : Flag<["-"], "nostop-on-failure">, Flags<[DriverOption]>;
+
 def working_directory : JoinedOrSeparate<["-"], "working-directory">, Flags<[CC1Option]>,
   HelpText<"Resolve file paths relative to the specified directory">;
 def working_directory_EQ : Joined<["-"], "working-directory=">, Flags<[CC1Option]>,
   Alias<working_directory>;
 
 // Double dash options, which are usually an alias for one of the previous
 // options.
 

lib/Driver/Compilation.cpp

static bool ActionFailed(const Action *A,
   return false;
 }
 
 static bool InputsOk(const Command &C,
                      const FailingCommandList &FailingCommands) {
   return !ActionFailed(&C.getSource(), FailingCommands);
 }
 
-void Compilation::ExecuteJobs(const JobList &Jobs,
+void Compilation::ExecuteJobs(const JobList &Jobs, bool StopOnFailure,
                               FailingCommandList &FailingCommands) const {
   for (const auto &Job : Jobs) {
     if (!InputsOk(Job, FailingCommands))
       continue;
     const Command *FailingCommand = nullptr;
-    if (int Res = ExecuteCommand(Job, FailingCommand))
+    if (int Res = ExecuteCommand(Job, FailingCommand)) {
       FailingCommands.push_back(std::make_pair(Res, FailingCommand));
+      if (StopOnFailure)
+        return;
+    }
   }
 }
 
 void Compilation::initCompilationForDiagnostics() {
   ForDiagnostics = true;
 
   // Free actions and jobs.
   Actions.clear();

lib/Driver/Driver.cpp

: Opts(createDriverOptTable()), Diags(Diags), VFS(VFS), Mode(GCCMode),
       SaveTemps(SaveTempsNone), LTOMode(LTOK_None),
       ClangExecutable(ClangExecutable),
       SysRoot(DEFAULT_SYSROOT), UseStdLib(true),
       DefaultTargetTriple(DefaultTargetTriple),
       DriverTitle("clang LLVM compiler"), CCPrintOptionsFilename(nullptr),
       CCPrintHeadersFilename(nullptr), CCLogDiagnosticsFilename(nullptr),
       CCCPrintBindings(false), CCPrintHeaders(false), CCLogDiagnostics(false),
       CCGenDiagnostics(false), CCCGenericGCCName(""), CheckInputsExist(true),
-      CCCUsePCH(true), SuppressMissingInputWarning(false) {
+      CCCUsePCH(true), SuppressMissingInputWarning(false),
+      StopOnJobFailure(false) {
 
   // Provide a sane fallback if no VFS is specified.
   if (!this->VFS)
     this->VFS = vfs::getRealFileSystem();
 
   Name = llvm::sys::path::filename(ClangExecutable);
   Dir = llvm::sys::path::parent_path(ClangExecutable);
   InstalledDir = Dir; // Provide a sensible default installed dir.
C->setCudaDeviceToolChain(
                                                    : "nvptx-nvidia-cuda")));
   if (!HandleImmediateArgs(*C))
     return C;
 
   // Construct the list of inputs.
   InputList Inputs;
   BuildInputs(C->getDefaultToolChain(), *TranslatedArgs, Inputs);
 
+  // StopOnJobFailure defaults to false, except for CUDA compilations.
+  if (Arg *A = C->getArgs().getLastArg(options::OPT_stop_on_failure,
+                                       options::OPT_nostop_on_failure))
+    StopOnJobFailure = A->getOption().matches(options::OPT_stop_on_failure);
+  else
+    StopOnJobFailure =
+        llvm::any_of(Inputs, [](const std::pair<types::ID, const Arg *> &I) {
+          return I.first == types::TY_CUDA;
+        });
+
   // Construct the list of abstract actions to perform for this compilation. On
   // MachO targets this uses the driver-driver and universal actions.
   if (TC.getTriple().isOSBinFormatMachO())
     BuildUniversalActions(*C, C->getDefaultToolChain(), Inputs);
   else
     BuildActions(*C, C->getDefaultToolChain(), C->getArgs(), Inputs,
                  C->getActions());
 
void Driver::generateCompilationDiagnostics(Compilation &C,
   if (Trap.hasErrorOccurred()) {
     Diag(clang::diag::note_drv_command_failed_diag_msg)
         << "Error generating preprocessed source(s).";
     return;
   }
 
   // Generate preprocessed output.
   SmallVector<std::pair<int, const Command *>, 4> FailingCommands;
-  C.ExecuteJobs(C.getJobs(), FailingCommands);
+  C.ExecuteJobs(C.getJobs(), /* StopOnFailure = */ false, FailingCommands);
 
   // If any of the preprocessing commands failed, clean up and exit.
   if (!FailingCommands.empty()) {
     if (!isSaveTempsEnabled())
       C.CleanupFileList(C.getTempFiles(), true);
 
     Diag(clang::diag::note_drv_command_failed_diag_msg)
         << "Error generating preprocessed source(s).";
int Driver::ExecuteCompilation(
   // If there were errors building the compilation, quit now.
   if (Diags.hasErrorOccurred())
     return 1;
 
   // Set up response file names for each command, if necessary
   for (auto &Job : C.getJobs())
     setUpResponseFiles(C, Job);
 
-  C.ExecuteJobs(C.getJobs(), FailingCommands);
+  C.ExecuteJobs(C.getJobs(), StopOnJobFailure, FailingCommands);
 
   // Remove temp files.
   C.CleanupFileList(C.getTempFiles());
 
   // If the command succeeded, we are done.
   if (FailingCommands.empty())
     return 0;