LGTM. I went ahead and committed with a few minor changes in r176198. Thanks, Matthew!
Chad On Feb 27, 2013, at 5:35 AM, Matthew Curtis <[email protected]> wrote: > How about the attached patch as a possible solution? > > It uses the dependency information in the Action associated with a Command to > prevent the execution of any Commands downstream from a failing Command. > > On 2/26/2013 3:27 PM, Chad Rosier wrote: >> >> On Feb 26, 2013, at 1:12 PM, Richard Smith <[email protected]> wrote: >> >>> On Tue, Feb 26, 2013 at 9:50 AM, Chad Rosier <[email protected]> wrote: >>> Matthew, >>> >>> On Feb 26, 2013, at 6:11 AM, Matthew Curtis <[email protected]> wrote: >>> >>>> Hello Chad, >>>> >>>> Could you provide a little more background on why it's important to >>>> execute subsequent commands after one fails? >>> >>> It is required for Unix conformance testing. :) >>> >>>> >>>> This change is currently causing some noise for failed compilations on >>>> hexagon: >>>> hello-world.c:17:26: error: expected ';' after expression >>>> printf("hello world\n") >>>> ^ >>>> ; >>>> 1 error generated. >>>> Assembler messages: >>>> Error: can't open /tmp/hello-world-ZhHzJ1.s for reading: No such file or >>>> directory >>>> <...>/qc/../gnu/bin/hexagon-ld: cannot find /tmp/hello-world-KKCeEQ.o: No >>>> such file or directory >>>> clang: error: hexagon-as command failed with exit code 1 (use -v to see >>>> invocation) >>>> clang: error: hexagon-ld command failed with exit code 1 (use -v to see >>>> invocation) >>>> >>>> Should it only apply to commands that do not depend on the output of a >>>> failing command? >>> >>> It would be nice to detect such dependencies, but I don't plan on >>> implementing it any time soon. >> >> Change of plan; I'll see if I can't get this fixed in a day or two. >> >>> >>>> >>>> I'm just trying to understand if this feature is perhaps not appropriate >>>> for the hexagon target or if I should try to do something to quiet or >>>> inhibit downstream commands. >>> >>> If it's a huge distraction, we might consider adding a driver flag that >>> would cause the previous behavior. >>> >>> This has made all failing multi-stage compiles ugly. Do we have a good >>> reason not to restore the previous behavior by default? This just looks >>> stupid: >>> >>> $ clang++ my_simple_program.cc >>> my_simple_program.cc:1:10: error: expected ';' after top level declarator >>> int error error; >>> ^ >>> ; >>> 1 error generated. >>> /usr/bin/ld: cannot find /tmp/--gWf0D2.o: No such file or directory >>> clang-3: error: linker command failed with exit code 1 (use -v to see >>> invocation)FWIW, the original idea was to prevent the failure of one >>> translation unit not effect >> >> FWIW, the original idea was to prevent the failure of one translation unit >> from preventing the compilation of another. Unfortunately, I missed the >> fact that the failure of any phase (e.g., preprocess, compile, assemble) for >> a single translation unit should prevent later phases from executing. >> >> Chad >> >>> >>> Chad >>> >>>> >>>> Thanks, >>>> Matthew Curtis >>>> >>>> On 1/29/2013 2:15 PM, Chad Rosier wrote: >>>>> Author: mcrosier >>>>> Date: Tue Jan 29 14:15:05 2013 >>>>> New Revision: 173825 >>>>> >>>>> URL: http://llvm.org/viewvc/llvm-project?rev=173825&view=rev >>>>> Log: >>>>> [driver] Refactor the driver so that a failing commands doesn't prevent >>>>> subsequent commands from being executed. >>>>> >>>>> The diagnostics generation isn't designed for this use case, so add a >>>>> note to >>>>> fix this in the very near future. For now, just generated the >>>>> diagnostics for >>>>> the first failing command. >>>>> Part of rdar://12984531 >>>>> >>>>> Modified: >>>>> cfe/trunk/include/clang/Driver/Compilation.h >>>>> cfe/trunk/include/clang/Driver/Driver.h >>>>> cfe/trunk/lib/Driver/Compilation.cpp >>>>> cfe/trunk/lib/Driver/Driver.cpp >>>>> cfe/trunk/test/Driver/output-file-cleanup.c >>>>> cfe/trunk/tools/driver/driver.cpp >>>>> >>>>> Modified: cfe/trunk/include/clang/Driver/Compilation.h >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Compilation.h?rev=173825&r1=173824&r2=173825&view=diff >>>>> ============================================================================== >>>>> --- cfe/trunk/include/clang/Driver/Compilation.h (original) >>>>> +++ cfe/trunk/include/clang/Driver/Compilation.h Tue Jan 29 14:15:05 2013 >>>>> @@ -175,10 +175,10 @@ public: >>>>> >>>>> /// ExecuteJob - Execute a single job. >>>>> /// >>>>> - /// \param FailingCommand - For non-zero results, this will be set to >>>>> the >>>>> - /// Command which failed. >>>>> - /// \return The accumulated result code of the job. >>>>> - int ExecuteJob(const Job &J, const Command *&FailingCommand) const; >>>>> + /// \param FailingCommands - For non-zero results, this will be a >>>>> vector of >>>>> + /// failing commands and their associated result code. >>>>> + void ExecuteJob(const Job &J, >>>>> + SmallVectorImpl< std::pair<int, const Command *> > >>>>> &FailingCommands) const; >>>>> >>>>> /// initCompilationForDiagnostics - Remove stale state and suppress >>>>> output >>>>> /// so compilation can be reexecuted to generate additional diagnostic >>>>> >>>>> Modified: cfe/trunk/include/clang/Driver/Driver.h >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Driver.h?rev=173825&r1=173824&r2=173825&view=diff >>>>> ============================================================================== >>>>> --- cfe/trunk/include/clang/Driver/Driver.h (original) >>>>> +++ cfe/trunk/include/clang/Driver/Driver.h Tue Jan 29 14:15:05 2013 >>>>> @@ -272,7 +272,7 @@ public: >>>>> /// to just running the subprocesses, for example reporting errors, >>>>> removing >>>>> /// temporary files, etc. >>>>> int ExecuteCompilation(const Compilation &C, >>>>> - const Command *&FailingCommand) const; >>>>> + SmallVectorImpl< std::pair<int, const Command *> > >>>>> &FailingCommands) const; >>>>> >>>>> /// generateCompilationDiagnostics - Generate diagnostics information >>>>> /// including preprocessed source file(s). >>>>> >>>>> Modified: cfe/trunk/lib/Driver/Compilation.cpp >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Compilation.cpp?rev=173825&r1=173824&r2=173825&view=diff >>>>> ============================================================================== >>>>> --- cfe/trunk/lib/Driver/Compilation.cpp (original) >>>>> +++ cfe/trunk/lib/Driver/Compilation.cpp Tue Jan 29 14:15:05 2013 >>>>> @@ -307,17 +307,17 @@ int Compilation::ExecuteCommand(const Co >>>>> return Res; >>>>> } >>>>> >>>>> -int Compilation::ExecuteJob(const Job &J, >>>>> - const Command *&FailingCommand) const { >>>>> +void Compilation::ExecuteJob(const Job &J, >>>>> + SmallVectorImpl< std::pair<int, const Command *> > &FailingCommands) >>>>> const { >>>>> if (const Command *C = dyn_cast<Command>(&J)) { >>>>> - return ExecuteCommand(*C, FailingCommand); >>>>> + const Command *FailingCommand = 0; >>>>> + if (int Res = ExecuteCommand(*C, FailingCommand)) >>>>> + FailingCommands.push_back(std::make_pair(Res, FailingCommand)); >>>>> } else { >>>>> const JobList *Jobs = cast<JobList>(&J); >>>>> - for (JobList::const_iterator >>>>> - it = Jobs->begin(), ie = Jobs->end(); it != ie; ++it) >>>>> - if (int Res = ExecuteJob(**it, FailingCommand)) >>>>> - return Res; >>>>> - return 0; >>>>> + for (JobList::const_iterator it = Jobs->begin(), ie = Jobs->end(); >>>>> + it != ie; ++it) >>>>> + ExecuteJob(**it, FailingCommands); >>>>> } >>>>> } >>>>> >>>>> >>>>> Modified: cfe/trunk/lib/Driver/Driver.cpp >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=173825&r1=173824&r2=173825&view=diff >>>>> ============================================================================== >>>>> --- cfe/trunk/lib/Driver/Driver.cpp (original) >>>>> +++ cfe/trunk/lib/Driver/Driver.cpp Tue Jan 29 14:15:05 2013 >>>>> @@ -440,11 +440,11 @@ void Driver::generateCompilationDiagnost >>>>> } >>>>> >>>>> // Generate preprocessed output. >>>>> - FailingCommand = 0; >>>>> - int Res = C.ExecuteJob(C.getJobs(), FailingCommand); >>>>> + SmallVector<std::pair<int, const Command *>, 4> FailingCommands; >>>>> + C.ExecuteJob(C.getJobs(), FailingCommands); >>>>> >>>>> // If the command succeeded, we are done. >>>>> - if (Res == 0) { >>>>> + if (FailingCommands.empty()) { >>>>> Diag(clang::diag::note_drv_command_failed_diag_msg) >>>>> << "\n********************\n\n" >>>>> "PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:\n" >>>>> @@ -495,7 +495,7 @@ void Driver::generateCompilationDiagnost >>>>> } >>>>> >>>>> int Driver::ExecuteCompilation(const Compilation &C, >>>>> - const Command *&FailingCommand) const { >>>>> + SmallVectorImpl< std::pair<int, const Command *> > &FailingCommands) >>>>> const { >>>>> // Just print if -### was present. >>>>> if (C.getArgs().hasArg(options::OPT__HASH_HASH_HASH)) { >>>>> C.PrintJob(llvm::errs(), C.getJobs(), "\n", true); >>>>> @@ -506,45 +506,52 @@ int Driver::ExecuteCompilation(const Com >>>>> if (Diags.hasErrorOccurred()) >>>>> return 1; >>>>> >>>>> - int Res = C.ExecuteJob(C.getJobs(), FailingCommand); >>>>> + C.ExecuteJob(C.getJobs(), FailingCommands); >>>>> >>>>> // Remove temp files. >>>>> C.CleanupFileList(C.getTempFiles()); >>>>> >>>>> // If the command succeeded, we are done. >>>>> - if (Res == 0) >>>>> - return Res; >>>>> + if (FailingCommands.empty()) >>>>> + return 0; >>>>> >>>>> - // Otherwise, remove result files as well. >>>>> - if (!C.getArgs().hasArg(options::OPT_save_temps)) { >>>>> - const JobAction *JA = cast<JobAction>(&FailingCommand->getSource()); >>>>> - C.CleanupFileMap(C.getResultFiles(), JA, true); >>>>> + // Otherwise, remove result files and print extra information about >>>>> abnormal >>>>> + // failures. >>>>> + for (SmallVectorImpl< std::pair<int, const Command *> >::iterator it = >>>>> + FailingCommands.begin(), ie = FailingCommands.end(); it != ie; >>>>> ++it) { >>>>> + int Res = it->first; >>>>> + const Command *FailingCommand = it->second; >>>>> >>>>> - // Failure result files are valid unless we crashed. >>>>> - if (Res < 0) >>>>> - C.CleanupFileMap(C.getFailureResultFiles(), JA, true); >>>>> - } >>>>> + // Remove result files if we're not saving temps. >>>>> + if (!C.getArgs().hasArg(options::OPT_save_temps)) { >>>>> + const JobAction *JA = >>>>> cast<JobAction>(&FailingCommand->getSource()); >>>>> + C.CleanupFileMap(C.getResultFiles(), JA, true); >>>>> >>>>> - // Print extra information about abnormal failures, if possible. >>>>> - // >>>>> - // This is ad-hoc, but we don't want to be excessively noisy. If the >>>>> result >>>>> - // status was 1, assume the command failed normally. In particular, if >>>>> it was >>>>> - // the compiler then assume it gave a reasonable error code. Failures >>>>> in other >>>>> - // tools are less common, and they generally have worse diagnostics, >>>>> so always >>>>> - // print the diagnostic there. >>>>> - const Tool &FailingTool = FailingCommand->getCreator(); >>>>> - >>>>> - if (!FailingCommand->getCreator().hasGoodDiagnostics() || Res != 1) { >>>>> - // FIXME: See FIXME above regarding result code interpretation. >>>>> - if (Res < 0) >>>>> - Diag(clang::diag::err_drv_command_signalled) >>>>> - << FailingTool.getShortName(); >>>>> - else >>>>> - Diag(clang::diag::err_drv_command_failed) >>>>> - << FailingTool.getShortName() << Res; >>>>> - } >>>>> + // Failure result files are valid unless we crashed. >>>>> + if (Res < 0) >>>>> + C.CleanupFileMap(C.getFailureResultFiles(), JA, true); >>>>> + } >>>>> >>>>> - return Res; >>>>> + // Print extra information about abnormal failures, if possible. >>>>> + // >>>>> + // This is ad-hoc, but we don't want to be excessively noisy. If the >>>>> result >>>>> + // status was 1, assume the command failed normally. In particular, >>>>> if it >>>>> + // was the compiler then assume it gave a reasonable error code. >>>>> Failures >>>>> + // in other tools are less common, and they generally have worse >>>>> + // diagnostics, so always print the diagnostic there. >>>>> + const Tool &FailingTool = FailingCommand->getCreator(); >>>>> + >>>>> + if (!FailingCommand->getCreator().hasGoodDiagnostics() || Res != 1) { >>>>> + // FIXME: See FIXME above regarding result code interpretation. >>>>> + if (Res < 0) >>>>> + Diag(clang::diag::err_drv_command_signalled) >>>>> + << FailingTool.getShortName(); >>>>> + else >>>>> + Diag(clang::diag::err_drv_command_failed) >>>>> + << FailingTool.getShortName() << Res; >>>>> + } >>>>> + } >>>>> + return 0; >>>>> } >>>>> >>>>> void Driver::PrintOptions(const ArgList &Args) const { >>>>> >>>>> Modified: cfe/trunk/test/Driver/output-file-cleanup.c >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/output-file-cleanup.c?rev=173825&r1=173824&r2=173825&view=diff >>>>> ============================================================================== >>>>> --- cfe/trunk/test/Driver/output-file-cleanup.c (original) >>>>> +++ cfe/trunk/test/Driver/output-file-cleanup.c Tue Jan 29 14:15:05 2013 >>>>> @@ -36,3 +36,15 @@ invalid C code >>>>> // RUN: cd %T && not %clang -S %t1.c %t2.c >>>>> // RUN: test -f %t1.s >>>>> // RUN: test ! -f %t2.s >>>>> + >>>>> +// RUN: touch %t1.c >>>>> +// RUN: echo "invalid C code" > %t2.c >>>>> +// RUN: touch %t3.c >>>>> +// RUN: echo "invalid C code" > %t4.c >>>>> +// RUN: touch %t5.c >>>>> +// RUN: cd %T && not %clang -S %t1.c %t2.c %t3.c %t4.c %t5.c >>>>> +// RUN: test -f %t1.s >>>>> +// RUN: test ! -f %t2.s >>>>> +// RUN: test -f %t3.s >>>>> +// RUN: test ! -f %t4.s >>>>> +// RUN: test -f %t5.s >>>>> >>>>> Modified: cfe/trunk/tools/driver/driver.cpp >>>>> URL: >>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/driver/driver.cpp?rev=173825&r1=173824&r2=173825&view=diff >>>>> ============================================================================== >>>>> --- cfe/trunk/tools/driver/driver.cpp (original) >>>>> +++ cfe/trunk/tools/driver/driver.cpp Tue Jan 29 14:15:05 2013 >>>>> @@ -466,21 +466,35 @@ int main(int argc_, const char **argv_) >>>>> >>>>> OwningPtr<Compilation> C(TheDriver.BuildCompilation(argv)); >>>>> int Res = 0; >>>>> - const Command *FailingCommand = 0; >>>>> + SmallVector<std::pair<int, const Command *>, 4> FailingCommands; >>>>> if (C.get()) >>>>> - Res = TheDriver.ExecuteCompilation(*C, FailingCommand); >>>>> + Res = TheDriver.ExecuteCompilation(*C, FailingCommands); >>>>> >>>>> // Force a crash to test the diagnostics. >>>>> if (::getenv("FORCE_CLANG_DIAGNOSTICS_CRASH")) { >>>>> Diags.Report(diag::err_drv_force_crash) << >>>>> "FORCE_CLANG_DIAGNOSTICS_CRASH"; >>>>> - Res = -1; >>>>> + const Command *FailingCommand = 0; >>>>> + FailingCommands.push_back(std::make_pair(-1, FailingCommand)); >>>>> } >>>>> >>>>> - // If result status is < 0, then the driver command signalled an error. >>>>> - // If result status is 70, then the driver command reported a fatal >>>>> error. >>>>> - // In these cases, generate additional diagnostic information if >>>>> possible. >>>>> - if (Res < 0 || Res == 70) >>>>> - TheDriver.generateCompilationDiagnostics(*C, FailingCommand); >>>>> + for (SmallVectorImpl< std::pair<int, const Command *> >::iterator it = >>>>> + FailingCommands.begin(), ie = FailingCommands.end(); it != ie; >>>>> ++it) { >>>>> + int CommandRes = it->first; >>>>> + const Command *FailingCommand = it->second; >>>>> + if (!Res) >>>>> + Res = CommandRes; >>>>> + >>>>> + // If result status is < 0, then the driver command signalled an >>>>> error. >>>>> + // If result status is 70, then the driver command reported a fatal >>>>> error. >>>>> + // In these cases, generate additional diagnostic information if >>>>> possible. >>>>> + if (CommandRes < 0 || CommandRes == 70) { >>>>> + TheDriver.generateCompilationDiagnostics(*C, FailingCommand); >>>>> + >>>>> + // FIXME: generateCompilationDiagnostics() needs to be tested when >>>>> there >>>>> + // are multiple failing commands. >>>>> + break; >>>>> + } >>>>> + } >>>>> >>>>> // If any timers were active but haven't been destroyed yet, print >>>>> their >>>>> // results now. This happens in -disable-free mode. >>>>> @@ -496,5 +510,7 @@ int main(int argc_, const char **argv_) >>>>> Res = 1; >>>>> #endif >>>>> >>>>> + // If we have multiple failing commands, we return the result of the >>>>> first >>>>> + // failing command. >>>>> return Res; >>>>> } >>>>> >>>>> >>>>> _______________________________________________ >>>>> cfe-commits mailing list >>>>> [email protected] >>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>>> >>>> -- >>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted >>>> by The Linux Foundation >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by > The Linux Foundation > <0001-Inhibit-downstream-commands-when-a-command-fails.patch>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
