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