r173825 - [driver] Refactor the driver so that a failing commands doesn't prevent
Matthew Curtis
mcurtis at codeaurora.org
Wed Feb 27 13:42:49 PST 2013
You're welcome. Glad I could help.
Matthew
On 2/27/2013 12:51 PM, Chad Rosier wrote:
> 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 <mcurtis at codeaurora.org
> <mailto:mcurtis at codeaurora.org>> 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 <richard at metafoo.co.uk
>>> <mailto:richard at metafoo.co.uk>> wrote:
>>>
>>>> On Tue, Feb 26, 2013 at 9:50 AM, Chad Rosier<mcrosier at apple.com
>>>> <mailto:mcrosier at apple.com>>wrote:
>>>>
>>>> Matthew,
>>>>
>>>> On Feb 26, 2013, at 6:11 AM, Matthew Curtis
>>>> <mcurtis at codeaurora.org <mailto:mcurtis at codeaurora.org>> 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 ofrdar://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
>>>>>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>>>>>> 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
>>>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>>>> 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>
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130227/9ede09f9/attachment.html>
More information about the cfe-commits
mailing list