(switching over to the new mailing list) On Wed, Aug 12, 2015 at 3:09 PM, David Blaikie <dblai...@gmail.com> wrote:
> > > On Tue, Oct 21, 2014 at 10:24 AM, Justin Bogner <m...@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? > > (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-comm...@cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
