Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Justin Lebar via cfe-commits]

jlebar added a comment.

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.


Repository:
  rL LLVM

http://reviews.llvm.org/D16514



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

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Justin Lebar via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL260448: Bail on compilation as soon as a job fails. 
(authored by jlebar).

Changed prior to commit:
  http://reviews.llvm.org/D16514?vs=47520=47525#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D16514

Files:
  cfe/trunk/lib/Driver/Compilation.cpp

Index: cfe/trunk/lib/Driver/Compilation.cpp
===
--- cfe/trunk/lib/Driver/Compilation.cpp
+++ cfe/trunk/lib/Driver/Compilation.cpp
@@ -163,39 +163,17 @@
   return ExecutionFailed ? 1 : Res;
 }
 
-typedef SmallVectorImpl< std::pair > FailingCommandList;
-
-static bool ActionFailed(const Action *A,
- const FailingCommandList ) {
-
-  if (FailingCommands.empty())
-return false;
-
-  for (FailingCommandList::const_iterator CI = FailingCommands.begin(),
- CE = FailingCommands.end(); CI != CE; ++CI)
-if (A == &(CI->second->getSource()))
-  return true;
-
-  for (Action::const_iterator AI = A->begin(), AE = A->end(); AI != AE; ++AI)
-if (ActionFailed(*AI, FailingCommands))
-  return true;
-
-  return false;
-}
-
-static bool InputsOk(const Command ,
- const FailingCommandList ) {
-  return !ActionFailed((), FailingCommands);
-}
-
-void Compilation::ExecuteJobs(const JobList ,
-  FailingCommandList ) const {
+void Compilation::ExecuteJobs(
+const JobList ,
+SmallVectorImpl> ) const {
   for (const auto  : 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));
+  // Bail as soon as one command fails, so we don't output duplicate error
+  // messages if we die on e.g. the same file.
+  return;
+}
   }
 }
 


Index: cfe/trunk/lib/Driver/Compilation.cpp
===
--- cfe/trunk/lib/Driver/Compilation.cpp
+++ cfe/trunk/lib/Driver/Compilation.cpp
@@ -163,39 +163,17 @@
   return ExecutionFailed ? 1 : Res;
 }
 
-typedef SmallVectorImpl< std::pair > FailingCommandList;
-
-static bool ActionFailed(const Action *A,
- const FailingCommandList ) {
-
-  if (FailingCommands.empty())
-return false;
-
-  for (FailingCommandList::const_iterator CI = FailingCommands.begin(),
- CE = FailingCommands.end(); CI != CE; ++CI)
-if (A == &(CI->second->getSource()))
-  return true;
-
-  for (Action::const_iterator AI = A->begin(), AE = A->end(); AI != AE; ++AI)
-if (ActionFailed(*AI, FailingCommands))
-  return true;
-
-  return false;
-}
-
-static bool InputsOk(const Command ,
- const FailingCommandList ) {
-  return !ActionFailed((), FailingCommands);
-}
-
-void Compilation::ExecuteJobs(const JobList ,
-  FailingCommandList ) const {
+void Compilation::ExecuteJobs(
+const JobList ,
+SmallVectorImpl> ) const {
   for (const auto  : 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));
+  // Bail as soon as one command fails, so we don't output duplicate error
+  // messages if we die on e.g. the same file.
+  return;
+}
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Eric Christopher via cfe-commits]

echristo accepted this revision.
echristo added a reviewer: echristo.
echristo added a comment.

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

Thanks!

-eric


http://reviews.llvm.org/D16514



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

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Justin Lebar via cfe-commits]

jlebar added a comment.

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.


http://reviews.llvm.org/D16514



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

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Justin Lebar via cfe-commits]

jlebar added a comment.

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.


http://reviews.llvm.org/D16514



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

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Justin Lebar via cfe-commits]

jlebar added a comment.

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


http://reviews.llvm.org/D16514



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

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Artem Belevich via cfe-commits]

tra added inline comments.


Comment at: include/clang/Driver/Options.td:1807
@@ +1806,3 @@
+"CUDA compilation without --save-temps.">;
+def nostop_on_failure : Flag<["-"], "nostop-on-failure">, 
Flags<[DriverOption]>;
+

I'd use 'no-' prefix.


Comment at: lib/Driver/Driver.cpp:650
@@ -638,3 +649,3 @@
   SmallVector, 4> FailingCommands;
-  C.ExecuteJobs(C.getJobs(), FailingCommands);
+  C.ExecuteJobs(C.getJobs(), /* StopOnFailure = */ false, FailingCommands);
 

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


http://reviews.llvm.org/D16514



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

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Justin Lebar via cfe-commits]

jlebar updated this revision to Diff 46300.
jlebar marked an inline comment as done.
jlebar added a comment.

Address tra's review comment (rename flag).


http://reviews.llvm.org/D16514

Files:
  include/clang/Driver/Compilation.h
  include/clang/Driver/Driver.h
  include/clang/Driver/Options.td
  lib/Driver/Compilation.cpp
  lib/Driver/Driver.cpp

Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -58,7 +58,8 @@
   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)
@@ -505,6 +506,16 @@
   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_no_stop_on_failure))
+StopOnJobFailure = A->getOption().matches(options::OPT_stop_on_failure);
+  else
+StopOnJobFailure =
+llvm::any_of(Inputs, [](const std::pair ) {
+  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())
@@ -638,7 +649,7 @@
 
   // Generate preprocessed output.
   SmallVector, 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()) {
@@ -730,7 +741,7 @@
   for (auto  : C.getJobs())
 setUpResponseFiles(C, Job);
 
-  C.ExecuteJobs(C.getJobs(), FailingCommands);
+  C.ExecuteJobs(C.getJobs(), StopOnJobFailure, FailingCommands);
 
   // Remove temp files.
   C.CleanupFileList(C.getTempFiles());
Index: lib/Driver/Compilation.cpp
===
--- lib/Driver/Compilation.cpp
+++ lib/Driver/Compilation.cpp
@@ -188,14 +188,17 @@
   return !ActionFailed((), FailingCommands);
 }
 
-void Compilation::ExecuteJobs(const JobList ,
+void Compilation::ExecuteJobs(const JobList , bool StopOnFailure,
   FailingCommandList ) const {
   for (const auto  : 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;
+}
   }
 }
 
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1801,6 +1801,11 @@
 def : Flag<["-"], "no-integrated-as">, Alias,
   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 no_stop_on_failure : Flag<["-"], "no-stop-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]>,
Index: include/clang/Driver/Driver.h
===
--- include/clang/Driver/Driver.h
+++ include/clang/Driver/Driver.h
@@ -192,6 +192,10 @@
   /// 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 TempFiles;
   std::list ResultFiles;
 
Index: include/clang/Driver/Compilation.h
===
--- include/clang/Driver/Compilation.h
+++ include/clang/Driver/Compilation.h
@@ -193,12 +193,13 @@
   /// \return The result code of the subprocess.
   int ExecuteCommand(const Command , const Command *) const;
 
-  /// ExecuteJob - Execute a single job.
+  /// ExecuteJobs - Execute a list of jobs.
   ///
-  /// \param FailingCommands - For non-zero results, this will be a vector of
-  /// 

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Justin Lebar via cfe-commits]

jlebar added inline comments.


Comment at: lib/Driver/Driver.cpp:650
@@ -638,3 +649,3 @@
   SmallVector, 4> FailingCommands;
-  C.ExecuteJobs(C.getJobs(), FailingCommands);
+  C.ExecuteJobs(C.getJobs(), /* StopOnFailure = */ false, FailingCommands);
 

tra wrote:
> Why is StopOnFailure is false in this case? Shouldn't it obey command line 
> options, too?
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?


http://reviews.llvm.org/D16514



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

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Justin Lebar via cfe-commits]

jlebar added inline comments.


Comment at: lib/Driver/Driver.cpp:652
@@ -640,3 +651,3 @@
   SmallVector, 4> FailingCommands;
-  C.ExecuteJobs(C.getJobs(), FailingCommands);
+  C.ExecuteJobs(C.getJobs(), /* StopOnFailure = */ false, FailingCommands);
 

tra wrote:
> jlebar wrote:
> > tra wrote:
> > > 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.
> > 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.
> 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.
> 
Oh.

Thanks.  :)


http://reviews.llvm.org/D16514



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

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Justin Lebar via cfe-commits]

jlebar updated this revision to Diff 46315.
jlebar marked 3 inline comments as done.
jlebar added a comment.

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


http://reviews.llvm.org/D16514

Files:
  include/clang/Driver/Compilation.h
  include/clang/Driver/Driver.h
  include/clang/Driver/Options.td
  lib/Driver/Compilation.cpp
  lib/Driver/Driver.cpp

Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -58,7 +58,8 @@
   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)
@@ -505,6 +506,16 @@
   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_no_stop_on_failure))
+StopOnJobFailure = A->getOption().matches(options::OPT_stop_on_failure);
+  else
+StopOnJobFailure =
+llvm::any_of(Inputs, [](const std::pair ) {
+  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())
@@ -638,7 +649,9 @@
 
   // Generate preprocessed output.
   SmallVector, 4> FailingCommands;
-  C.ExecuteJobs(C.getJobs(), FailingCommands);
+  // Might as well pass StopOnFailure = true; if any of the commands fails, we
+  // don't output anything at all.
+  C.ExecuteJobs(C.getJobs(), /* StopOnFailure = */ true, FailingCommands);
 
   // If any of the preprocessing commands failed, clean up and exit.
   if (!FailingCommands.empty()) {
@@ -730,7 +743,7 @@
   for (auto  : C.getJobs())
 setUpResponseFiles(C, Job);
 
-  C.ExecuteJobs(C.getJobs(), FailingCommands);
+  C.ExecuteJobs(C.getJobs(), StopOnJobFailure, FailingCommands);
 
   // Remove temp files.
   C.CleanupFileList(C.getTempFiles());
Index: lib/Driver/Compilation.cpp
===
--- lib/Driver/Compilation.cpp
+++ lib/Driver/Compilation.cpp
@@ -188,14 +188,17 @@
   return !ActionFailed((), FailingCommands);
 }
 
-void Compilation::ExecuteJobs(const JobList ,
+void Compilation::ExecuteJobs(const JobList , bool StopOnFailure,
   FailingCommandList ) const {
   for (const auto  : 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;
+}
   }
 }
 
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1801,6 +1801,11 @@
 def : Flag<["-"], "no-integrated-as">, Alias,
   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 no_stop_on_failure : Flag<["-"], "no-stop-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]>,
Index: include/clang/Driver/Driver.h
===
--- include/clang/Driver/Driver.h
+++ include/clang/Driver/Driver.h
@@ -192,6 +192,10 @@
   /// 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 TempFiles;
   std::list ResultFiles;
 
Index: include/clang/Driver/Compilation.h
===
--- include/clang/Driver/Compilation.h
+++ include/clang/Driver/Compilation.h
@@ -193,12 +193,13 @@
   /// \return The result code of the subprocess.
   int ExecuteCommand(const Command , const Command *) const;
 
-  /// ExecuteJob - Execute a single job.
+  

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Eric Christopher via cfe-commits]

echristo added a comment.



> 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


http://reviews.llvm.org/D16514



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

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Eric Christopher via cfe-commits]

echristo added a comment.

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


http://reviews.llvm.org/D16514



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

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Artem Belevich via cfe-commits]

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

LGTM.



Comment at: lib/Driver/Driver.cpp:652
@@ -640,3 +651,3 @@
   SmallVector, 4> FailingCommands;
-  C.ExecuteJobs(C.getJobs(), FailingCommands);
+  C.ExecuteJobs(C.getJobs(), /* StopOnFailure = */ false, FailingCommands);
 

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.


http://reviews.llvm.org/D16514



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

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Justin Lebar via cfe-commits]

jlebar added a comment.

In http://reviews.llvm.org/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.



Comment at: lib/Driver/Driver.cpp:652
@@ -640,3 +651,3 @@
   SmallVector, 4> FailingCommands;
-  C.ExecuteJobs(C.getJobs(), FailingCommands);
+  C.ExecuteJobs(C.getJobs(), /* StopOnFailure = */ false, FailingCommands);
 

tra wrote:
> 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.
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.


http://reviews.llvm.org/D16514



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

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Artem Belevich via cfe-commits]

tra added inline comments.


Comment at: lib/Driver/Driver.cpp:652
@@ -640,3 +651,3 @@
   SmallVector, 4> FailingCommands;
-  C.ExecuteJobs(C.getJobs(), FailingCommands);
+  C.ExecuteJobs(C.getJobs(), /* StopOnFailure = */ false, FailingCommands);
 

jlebar wrote:
> tra wrote:
> > 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.
> 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.
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.



http://reviews.llvm.org/D16514



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

Re: [PATCH] D16514: Add -stop-on-failure driver option, and enable it by default for CUDA compiles.

[Justin Lebar via cfe-commits]

jlebar added a comment.

Friendly ping.


http://reviews.llvm.org/D16514



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