r220305 - Driver: Move crash report command mangling into Command::Print
David Blaikie via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 19 13:22:40 PDT 2015
On Wed, Aug 12, 2015 at 5:00 PM, Justin Bogner <mail at justinbogner.com>
wrote:
> 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.
>
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.
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.
>
>
>
> > (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
> >>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150819/a5a3ffa2/attachment-0001.html>
More information about the cfe-commits
mailing list