r220305 - Driver: Move crash report command mangling into Command::Print

David Blaikie via cfe-commits cfe-commits at lists.llvm.org
Wed Aug 12 15:10:24 PDT 2015


(switching over to the new mailing list)

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?
>
> (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/20150812/031d37e7/attachment-0001.html>


More information about the cfe-commits mailing list