r220305 - Driver: Move crash report command mangling into Command::Print
Justin Bogner via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 12 17:00:16 PDT 2015
On Wed, Aug 12, 2015 at 3:09 PM, David Blaikie <dblaikie at gmail.com> wrote:
> On Tue, Oct 21, 2014 at 10:24 AM, Justin Bogner <mail at justinbogner.com>
> wrote:
>> Author: bogner
>> Date: Tue Oct 21 12:24:44 2014
>> New Revision: 220305
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=220305&view=rev
>> Log:
>> Driver: Move crash report command mangling into Command::Print
>>
>> This pushes the logic for generating a crash reproduction script
>> entirely into Command::Print, instead of Command doing half of the
>> work and then relying on textual substitution for the rest. This makes
>> this logic much easier to read and will simplify fixing a couple of
>> issues in this area.
>
>
>> Modified:
>> cfe/trunk/include/clang/Driver/Job.h
>> cfe/trunk/lib/Driver/Driver.cpp
>> cfe/trunk/lib/Driver/Job.cpp
>>
>> Modified: cfe/trunk/include/clang/Driver/Job.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Job.h?rev=220305&r1=220304&r2=220305&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/include/clang/Driver/Job.h (original)
>> +++ cfe/trunk/include/clang/Driver/Job.h Tue Oct 21 12:24:44 2014
>> @@ -29,6 +29,14 @@ class Tool;
>> // Re-export this as clang::driver::ArgStringList.
>> using llvm::opt::ArgStringList;
>>
>> +struct CrashReportInfo {
>> + StringRef Filename;
>> + StringRef VFSPath;
>> +
>> + CrashReportInfo(StringRef Filename, StringRef VFSPath)
>> + : Filename(Filename), VFSPath(VFSPath) {}
>> +};
>> +
>> class Job {
>> public:
>> enum JobClass {
>> @@ -52,9 +60,9 @@ public:
>> /// \param OS - The stream to print on.
>> /// \param Terminator - A string to print at the end of the line.
>> /// \param Quote - Should separate arguments be quoted.
>> - /// \param CrashReport - Whether to print for inclusion in a crash
>> report.
>> - virtual void Print(llvm::raw_ostream &OS, const char *Terminator,
>> - bool Quote, bool CrashReport = false) const = 0;
>> + /// \param CrashInfo - Details for inclusion in a crash report.
>> + virtual void Print(llvm::raw_ostream &OS, const char *Terminator, bool
>> Quote,
>> + CrashReportInfo *CrashInfo = nullptr) const = 0;
>> };
>>
>> /// Command - An executable path/name and argument vector to
>> @@ -102,7 +110,7 @@ public:
>> const llvm::opt::ArgStringList &_Arguments);
>>
>> void Print(llvm::raw_ostream &OS, const char *Terminator, bool Quote,
>> - bool CrashReport = false) const override;
>> + CrashReportInfo *CrashInfo = nullptr) const override;
>>
>> virtual int Execute(const StringRef **Redirects, std::string *ErrMsg,
>> bool *ExecutionFailed) const;
>> @@ -141,7 +149,7 @@ public:
>> std::unique_ptr<Command> Fallback_);
>>
>> void Print(llvm::raw_ostream &OS, const char *Terminator, bool Quote,
>> - bool CrashReport = false) const override;
>> + CrashReportInfo *CrashInfo = nullptr) const override;
>>
>> int Execute(const StringRef **Redirects, std::string *ErrMsg,
>> bool *ExecutionFailed) const override;
>> @@ -170,7 +178,7 @@ public:
>> virtual ~JobList() {}
>>
>> void Print(llvm::raw_ostream &OS, const char *Terminator,
>> - bool Quote, bool CrashReport = false) const override;
>> + bool Quote, CrashReportInfo *CrashInfo = nullptr) const
>> override;
>>
>> /// Add a job to the list (taking ownership).
>> void addJob(std::unique_ptr<Job> J) { Jobs.push_back(std::move(J)); }
>>
>> Modified: cfe/trunk/lib/Driver/Driver.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=220305&r1=220304&r2=220305&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Driver/Driver.cpp (original)
>> +++ cfe/trunk/lib/Driver/Driver.cpp Tue Oct 21 12:24:44 2014
>> @@ -424,10 +424,7 @@ void Driver::generateCompilationDiagnost
>> CCGenDiagnostics = true;
>>
>> // Save the original job command(s).
>> - std::string Cmd;
>> - llvm::raw_string_ostream OS(Cmd);
>> - FailingCommand.Print(OS, "\n", /*Quote*/ false, /*CrashReport*/ true);
>> - OS.flush();
>> + Command Cmd = FailingCommand;
>>
>
> It seems this code was/is undertested, or it would've caught the bug that
> I believe was introduced here.
>
> This line of code ("Command Cmd = FailingCommand") silently slices the
> FailingCommand object into a plain Command object, even in cases where
> FailingCommand is a derived class. This means that later on when
> "Cmd.Print" is called, it only prints as a plain Command, not as an
> appropriately derived Command object.
>
> Could you add more testing here to demonstrate this bug, and perhaps
> revert to the original code or otherwise address this issue?
Surprisingly, it's almost impossible to actually hit this bug, and the
behaviour with the sliced object actually ends being as good or better
than the behaviour would be without. It is definitely a bug though.
The only subclass of Command is FallbackCommand, which is only used when
the driver is in "cl" mode, and causes us to try MSVC's cl.exe instead
if clang fails. This means the only way to actually get to this code
path is if clang crashes and then cl.exe crashes *too*. If that does
happen, the slicing will cause us to print "clang -cc1 ... foo.c"
instead of "clang -cc1 ... foo.c || cl.exe ... foo.c" in the crash
reproduction script, which is fine because the clang command is what
we actually want to debug.
The attached test for clang-cl crash dumps passes, but it's a bit silly
since it requires "shell" and is only really interesting on windows. You
can change "%clang_cl /Zs" to "%clang_cl /c /fallback" to get the
fallback command to run, but unless you can force it to crash it isn't
really interesting. I'm not sure if it's worth committing.
I'll look into whether I can rework the "clang-cl /fallback" flow to
avoid needing a subclass of Command - I think that would be the cleanest
way to deal with this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: crash-report-clang-cl.patch
Type: text/x-patch
Size: 1643 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150812/fdba456f/attachment-0001.bin>
-------------- next part --------------
> (this came up while I was trying to enable -Wdeprecated in LLVM and it
> pointed to the call to the deprecated implicit copy constructor for Command
> (it's deprecated in the presence of a user-declared dtor on the class))
>
>
>>
>> // Keep track of whether we produce any errors while trying to produce
>> // preprocessed sources.
>> @@ -541,36 +538,16 @@ void Driver::generateCompilationDiagnost
>> }
>>
>> // Assume associated files are based off of the first temporary file.
>> - const char *MainFile = TempFiles[0];
>> + CrashReportInfo CrashInfo(TempFiles[0], VFS);
>>
>> - std::string Script = StringRef(MainFile).rsplit('.').first.str() +
>> ".sh";
>> + std::string Script = CrashInfo.Filename.rsplit('.').first.str() +
>> ".sh";
>> std::error_code EC;
>> llvm::raw_fd_ostream ScriptOS(Script, EC, llvm::sys::fs::F_Excl);
>> if (EC) {
>> Diag(clang::diag::note_drv_command_failed_diag_msg)
>> << "Error generating run script: " + Script + " " + EC.message();
>> } else {
>> - // Replace the original filename with the preprocessed one.
>> - size_t I, E;
>> - I = Cmd.find("-main-file-name ");
>> - assert(I != std::string::npos && "Expected to find -main-file-name");
>> - I += 16;
>> - E = Cmd.find(" ", I);
>> - assert(E != std::string::npos && "-main-file-name missing
>> argument?");
>> - StringRef OldFilename = StringRef(Cmd).slice(I, E);
>> - StringRef NewFilename = llvm::sys::path::filename(MainFile);
>> - I = StringRef(Cmd).rfind(OldFilename);
>> - E = I + OldFilename.size();
>> - if (E + 1 < Cmd.size() && Cmd[E] == '"')
>> - ++E; // Replace a trailing quote if present.
>> - I = Cmd.rfind(" ", I) + 1;
>> - Cmd.replace(I, E - I, NewFilename.data(), NewFilename.size());
>> - if (!VFS.empty()) {
>> - // Add the VFS overlay to the reproduction script.
>> - I += NewFilename.size();
>> - Cmd.insert(I, std::string(" -ivfsoverlay ") + VFS.c_str());
>> - }
>> - ScriptOS << Cmd;
>> + Cmd.Print(ScriptOS, "\n", /*Quote=*/false, &CrashInfo);
>> Diag(clang::diag::note_drv_command_failed_diag_msg) << Script;
>> }
>> Diag(clang::diag::note_drv_command_failed_diag_msg)
>>
>> Modified: cfe/trunk/lib/Driver/Job.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Job.cpp?rev=220305&r1=220304&r2=220305&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Driver/Job.cpp (original)
>> +++ cfe/trunk/lib/Driver/Job.cpp Tue Oct 21 12:24:44 2014
>> @@ -151,7 +151,7 @@ void Command::buildArgvForResponseFile(
>> }
>>
>> void Command::Print(raw_ostream &OS, const char *Terminator, bool Quote,
>> - bool CrashReport) const {
>> + CrashReportInfo *CrashInfo) const {
>> // Always quote the exe.
>> OS << ' ';
>> PrintArg(OS, Executable, /*Quote=*/true);
>> @@ -163,25 +163,46 @@ void Command::Print(raw_ostream &OS, con
>> Args = ArrayRef<const char *>(ArgsRespFile).slice(1); // no
>> executable name
>> }
>>
>> + StringRef MainFilename;
>> + // We'll need the argument to -main-file-name to find the input file
>> name.
>> + if (CrashInfo)
>> + for (size_t I = 0, E = Args.size(); I + 1 < E; ++I)
>> + if (StringRef(Args[I]).equals("-main-file-name"))
>> + MainFilename = Args[I + 1];
>> +
>> for (size_t i = 0, e = Args.size(); i < e; ++i) {
>> const char *const Arg = Args[i];
>>
>> - if (CrashReport) {
>> + if (CrashInfo) {
>> if (int Skip = skipArgs(Arg)) {
>> i += Skip - 1;
>> continue;
>> + } else if (llvm::sys::path::filename(Arg) == MainFilename &&
>> + (i == 0 || StringRef(Args[i - 1]) !=
>> "-main-file-name")) {
>> + // Replace the input file name with the crashinfo's file name.
>> + OS << ' ';
>> + StringRef ShortName =
>> llvm::sys::path::filename(CrashInfo->Filename);
>> + PrintArg(OS, ShortName.str().c_str(), Quote);
>> + continue;
>> }
>> }
>>
>> OS << ' ';
>> PrintArg(OS, Arg, Quote);
>>
>> - if (CrashReport && quoteNextArg(Arg) && i + 1 < e) {
>> + if (CrashInfo && quoteNextArg(Arg) && i + 1 < e) {
>> OS << ' ';
>> PrintArg(OS, Args[++i], true);
>> }
>> }
>>
>> + if (CrashInfo && !CrashInfo->VFSPath.empty()) {
>> + OS << ' ';
>> + PrintArg(OS, "-ivfsoverlay", Quote);
>> + OS << ' ';
>> + PrintArg(OS, CrashInfo->VFSPath.str().c_str(), Quote);
>> + }
>> +
>> if (ResponseFile != nullptr) {
>> OS << "\n Arguments passed via response file:\n";
>> writeResponseFile(OS);
>> @@ -251,10 +272,10 @@ FallbackCommand::FallbackCommand(const A
>> Fallback(std::move(Fallback_)) {}
>>
>> void FallbackCommand::Print(raw_ostream &OS, const char *Terminator,
>> - bool Quote, bool CrashReport) const {
>> - Command::Print(OS, "", Quote, CrashReport);
>> + bool Quote, CrashReportInfo *CrashInfo)
>> const {
>> + Command::Print(OS, "", Quote, CrashInfo);
>> OS << " ||";
>> - Fallback->Print(OS, Terminator, Quote, CrashReport);
>> + Fallback->Print(OS, Terminator, Quote, CrashInfo);
>> }
>>
>> static bool ShouldFallback(int ExitCode) {
>> @@ -286,9 +307,9 @@ int FallbackCommand::Execute(const Strin
>> JobList::JobList() : Job(JobListClass) {}
>>
>> void JobList::Print(raw_ostream &OS, const char *Terminator, bool Quote,
>> - bool CrashReport) const {
>> + CrashReportInfo *CrashInfo) const {
>> for (const auto &Job : *this)
>> - Job.Print(OS, Terminator, Quote, CrashReport);
>> + Job.Print(OS, Terminator, Quote, CrashInfo);
>> }
>>
>> void JobList::clear() { Jobs.clear(); }
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
More information about the cfe-commits
mailing list