[Clang] After integrated-cc1, ignore -disable-free when there are more than one job in the queue

[aganea]

Summary

A CC1Command was previously always burying pointers (ie. skipping object deletion). This lead to higher memory usage scenarios, when the inputs or cmd-line flags expand to more than one driver job.

With this patch, when several jobs are created by the driver, and at least one CC1Command is in the job queue, remove -disable-free for CC1Commands other than the last one.
If the last job is not a CC1Command, this patch removes -disable-free altogether for all CC1Commands.

Fixes PR44823.

[aganea]

Wording.

[rnk]

clang/include/clang/Driver/Job.h

57–58

Pack a new field after an existing bool for maximum micro optimization. :)

138

I guess I lean towards making this a field, default initialized to false, set to true in the CC1Command constructor. Less relocations, etc.

clang/lib/Driver/Driver.cpp

3759

*std::prev(Jobs.end())? Or *Jobs.getJobs().back(), not quite sure.

[aganea]

As suggested by Reid.

[hans]

clang/include/clang/Driver/Job.h

90

I think Reid just meant put the bool fields after each other to minimize padding. Using bitfields is taking it one step further. I think in LLVM we generally use "unsigned" for bitfield types, but I'm not sure using bitfields at all is worth it here.

134–135

I hadn't heard the term "burying pointers" before, and the name is also pretty broad: "clean up" could refer to a lot more than just freeing memory.

Maybe "enableFree()" or something would be a better name?

clang/lib/Driver/Driver.cpp

3758

Is this if-statement really necessary? If you just do the for-loop directly, it will not execute if Jobs is empty anyway.

Also, with an old-school for-loop it's easier to not iterate over the last element. How about something like:

for (size_t i = 0, e = C.getJobs().size(); i + 1 < e; i++) {
  auto &Job = C.getJobs().getJobs()[i];
  if (Job.InProcess)
    Job.forceCleanup();
}

clang/lib/Driver/Job.cpp

325

I wish it were possible to avoid adding the -disable-free flag to in-process-cc1 jobs in the first place, but I guess maybe that's not practical.

[thakis]

Isn't a better approach to only do in-process cc1 if there's just one TU?

[aganea]

In D74447#1872043, @thakis wrote:

Isn't a better approach to only do in-process cc1 if there's just one TU?

One of the reasons of the in-process cc1 was the debuggability. If we can compile several TUs in-process, why not doing it?

As for build times, we found that even with the clean up, in-process+clean-up is faster than out-of-process+disable-free. That is, on Windows and in the context of the llvm-buildozer app I've presented at the LLVM conf., where all the TUs for a project are compiled in the same process (meaning there's a clean up after each TU).

Any other issues you were seeing with this current approach?

[thakis]

I'd think that everyone debugging clang always passes a single TU to it, so I'm not sure debuggability does much here :)

The -disable-free code has never been used in normal compilations, so we didn't have to worry about this path. This patch here brings us to 3 modes (in-process cc1 with and without disable-free, out-of-process cc1). My suggestion keeps us to two modes (in-process cc1, out-of-process cc1).

[erichkeane]

The performance benefit of disable-free is still valuable, I'd not want to make every TU pay the price for a multiple TU invocation. Additionally, if we're willing to put up with the 3rd form (which i think is worth it), the multiple TU case cleaning up on all but the last makes sense.

[aganea]

Address @hans' comments.

While it'd be good to have a bot running without -disable-free, I concur with @thakis. This patch will probably introduce too much (untested) variability for release 10.0, if merged.
Let's enable integrate-cc1 just for one TU for the 10.0, and then we'll see if we want to land this patch or not in the trunk for the 11.0.
I'll open another review.

clang/include/clang/Driver/Job.h

90

Reid said "pack" and "field" and my Pavlovian reflex made me think "bitfields". Yes, plain bools are just fine.

134–135

"burying", as in https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/BuryPointer.h

Changed the comment and the function to enableFree().

clang/lib/Driver/Job.cpp

325

That was my original intent, but the -disable-free flag is added by Clang::ConstructJob() and at that point we don't know yet how many jobs we will have, nor their nature.

[thakis]

I forgot another reason why "only do cc1 with a single TU" might be a good idea: In case crash recovery turns out to not work in all cases with in-process cc1, we could add a signal handler that on crash make the driver exec() itself with -fno-integrated-cc1 added, so that the crash is then processed again with out-of-process cc1. (At least on non-Windows.) I prototyped this at https://github.com/nico/hack/blob/master/crash_signal.cc a while ago (in a standalone program). This only works if the crashing cc1 invocation is the first thing the driver does.

[ychen]

In D74447#1872211, @thakis wrote:

I'd think that everyone debugging clang always passes a single TU to it, so I'm not sure debuggability does much here :)

The -disable-free code has never been used in normal compilations, so we didn't have to worry about this path. This patch here brings us to 3 modes (in-process cc1 with and without disable-free, out-of-process cc1). My suggestion keeps us to two modes (in-process cc1, out-of-process cc1).

What is the downside of 3 modes except maintaining one more mode in the future?

clang/include/clang/Driver/Job.h

class Command {
   const char *Executable;
 
   /// The list of program arguments (not including the implicit first
   /// argument, which will be the executable).
   llvm::opt::ArgStringList Arguments;
 
   /// The list of program arguments which are inputs.
   llvm::opt::ArgStringList InputFilenames;
 
-  /// Whether to print the input filenames when executing.
-  bool PrintInputFilenames = false;
-
   /// Response file name, if this command is set to use one, or nullptr
   /// otherwise
   const char *ResponseFile = nullptr;
 
   /// The input file list in case we need to emit a file list instead of a
   /// proper response file
   llvm::opt::ArgStringList InputFileList;
 
   /// String storage if we need to create a new argument to specify a response
class Command {
 
   /// Encodes an array of C strings into a single string separated by whitespace.
   /// This function will also put in quotes arguments that have whitespaces and
   /// will escape the regular backslashes (used in Windows paths) and quotes.
   /// The results are the contents of a response file, written into a raw_ostream.
   void writeResponseFile(raw_ostream &OS) const;
 
 public:
+  /// Whether to print the input filenames when executing.
+  bool PrintInputFilenames;
+
+  /// Whether the command will be executed in this process or not.
+  bool InProcess;
+
   Command(const Action &Source, const Tool &Creator, const char *Executable,
           const llvm::opt::ArgStringList &Arguments,
           ArrayRef<InputInfo> Inputs);
   // FIXME: This really shouldn't be copyable, but is currently copied in some
   // error handling in Driver::generateCompilationDiagnostics.
   Command(const Command &) = default;
   virtual ~Command() = default;
 
public:
 
   const char *getExecutable() const { return Executable; }
 
   const llvm::opt::ArgStringList &getArguments() const { return Arguments; }
 
   /// Print a command argument, and optionally quote it.
   static void printArg(llvm::raw_ostream &OS, StringRef Arg, bool Quote);
 
-  /// Set whether to print the input filenames when executing.
-  void setPrintInputFilenames(bool P) { PrintInputFilenames = P; }
+  /// Ensure the memory allocated by the job is freed after execution.
+  void enableFree();
 
 protected:
   /// Optionally print the filenames to be compiled
   void PrintFileNames() const;
 };
 
 /// Use the CC1 tool callback when available, to avoid creating a new process
 class CC1Command : public Command {
 public:
-  using Command::Command;
+  CC1Command(const Action &Source, const Tool &Creator, const char *Executable,
+             const llvm::opt::ArgStringList &Arguments,
+             ArrayRef<InputInfo> Inputs);
 
   void Print(llvm::raw_ostream &OS, const char *Terminator, bool Quote,
              CrashReportInfo *CrashInfo = nullptr) const override;
 
   int Execute(ArrayRef<Optional<StringRef>> Redirects, std::string *ErrMsg,
               bool *ExecutionFailed) const override;
 
   void setEnvironment(llvm::ArrayRef<const char *> NewEnvironment) override;

clang/lib/Driver/Driver.cpp

for (Action *A : C.getActions()) {
     BuildJobsForAction(C, A, &C.getDefaultToolChain(),
                        /*BoundArch*/ StringRef(),
                        /*AtTopLevel*/ true,
                        /*MultipleArchs*/ ArchNames.size() > 1,
                        /*LinkingOutput*/ LinkingOutput, CachedResults,
                        /*TargetDeviceOffloadKind*/ Action::OFK_None);
   }
 
+  // Make sure we clean up after in-process jobs.
+  for (size_t I = 0, E = C.getJobs().size(); I + 1 < E; I++) {
+    auto &Job = *C.getJobs().getJobs()[I];
+    if (Job.InProcess)
+      Job.enableFree();
+  }
+
   // If the user passed -Qunused-arguments or there were errors, don't warn
   // about any unused arguments.
   if (Diags.hasErrorOccurred() ||
       C.getArgs().hasArg(options::OPT_Qunused_arguments))
     return;
 
   // Claim -### here.
   (void)C.getArgs().hasArg(options::OPT__HASH_HASH_HASH);

clang/lib/Driver/Job.cpp

 using namespace clang;
 using namespace driver;
 
 Command::Command(const Action &Source, const Tool &Creator,
                  const char *Executable,
                  const llvm::opt::ArgStringList &Arguments,
                  ArrayRef<InputInfo> Inputs)
     : Source(Source), Creator(Creator), Executable(Executable),
-      Arguments(Arguments) {
+      Arguments(Arguments), PrintInputFilenames(false), InProcess(false) {
   for (const auto &II : Inputs)
     if (II.isFilename())
       InputFilenames.push_back(II.getFilename());
 }
 
 /// Check if the compiler flag in question should be skipped when
 /// emitting a reproducer. Also track how many arguments it has and if the
 /// option is some kind of include path.

 }
 
 void Command::setEnvironment(llvm::ArrayRef<const char *> NewEnvironment) {
   Environment.reserve(NewEnvironment.size() + 1);
   Environment.assign(NewEnvironment.begin(), NewEnvironment.end());
   Environment.push_back(nullptr);
 }
 
+// In some cases when running the calling process, we need to clean up if there
+// are other Commands being executed after us, to prevent bloating the heap.
+void Command::enableFree() {
+  auto RemoveDisableFree = [](const char *A) {
+    return StringRef(A).equals("-disable-free");
+  };
+  llvm::erase_if(Arguments, RemoveDisableFree);
+}
+
 void Command::PrintFileNames() const {
   if (PrintInputFilenames) {
     for (const char *Arg : InputFilenames)
       llvm::outs() << llvm::sys::path::filename(Arg) << "\n";
     llvm::outs().flush();
   }
 }
 
int Command::Execute(ArrayRef<llvm::Optional<StringRef>> Redirects,
   }
 
   auto Args = llvm::toStringRefArray(Argv.data());
   return llvm::sys::ExecuteAndWait(Executable, Args, Env, Redirects,
                                    /*secondsToWait*/ 0,
                                    /*memoryLimit*/ 0, ErrMsg, ExecutionFailed);
 }
 
+CC1Command::CC1Command(const Action &Source, const Tool &Creator,
+                       const char *Executable,
+                       const llvm::opt::ArgStringList &Arguments,
+                       ArrayRef<InputInfo> Inputs)
+    : Command(Source, Creator, Executable, Arguments, Inputs) {
+  InProcess = true;
+}
+
 void CC1Command::Print(raw_ostream &OS, const char *Terminator, bool Quote,
                        CrashReportInfo *CrashInfo) const {
   OS << " (in-process)\n";
   Command::Print(OS, Terminator, Quote, CrashInfo);
 }
 
 int CC1Command::Execute(ArrayRef<llvm::Optional<StringRef>> /*Redirects*/,
                         std::string *ErrMsg, bool *ExecutionFailed) const {

clang/lib/Driver/ToolChains/Clang.cpp

   } else {
     C.addCommand(std::make_unique<Command>(JA, *this, Exec, CmdArgs, Inputs));
   }
 
   // Make the compile command echo its inputs for /showFilenames.
   if (Output.getType() == types::TY_Object &&
       Args.hasFlag(options::OPT__SLASH_showFilenames,
                    options::OPT__SLASH_showFilenames_, false)) {
-    C.getJobs().getJobs().back()->setPrintInputFilenames(true);
+    C.getJobs().getJobs().back()->PrintInputFilenames = true;
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
     if (FPKeepKind == CodeGenOptions::FramePointerKind::None)
       D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
                                                       << A->getAsString(Args);
 
   // Claim some arguments which clang supports automatically.

clang/test/Driver/cc1-spawnprocess.c

 // RUN:     %clang -fintegrated-cc1 -### %s 2>&1 \
 // RUN:     | FileCheck %s --check-prefix=YES
 // RUN: env CCC_OVERRIDE_OPTIONS=+-fno-integrated-cc1 \
 // RUN:     %clang -fintegrated-cc1 -### %s 2>&1 \
 // RUN:     | FileCheck %s --check-prefix=NO
 
 // YES: (in-process)
 // NO-NOT: (in-process)
+
+// The following tests ensure that only the last integrated-cc1 has -disable-free
+
+// RUN: echo 'int main() { return f() + g(); }' > %t1.cpp
+// RUN: echo 'int f() { return 1; }' > %t2.cpp
+// RUN: echo 'int g() { return 2; }' > %t3.cpp
+
+// Only one TU, one job, thus no cleanup is needed.
+// RUN: %clang -fintegrated-cc1 -c %t1.cpp -### 2>&1 | FileCheck %s --check-prefix=CLEAN
+// CLEAN: cc1
+// CLEAN-SAME: -disable-free
+
+// Three jobs, only the last invocation will skip clean up.
+// RUN: %clang -fintegrated-cc1 -c %t1.cpp %t2.cpp %t3.cpp -### 2>&1 | FileCheck %s --check-prefix=NOCLEAN-LAST
+// NOCLEAN-LAST: cc1
+// NOCLEAN-LAST-NOT: -disable-free
+// NOCLEAN-LAST-SAME: {{.*}}1.cpp
+// NOCLEAN-LAST: cc1
+// NOCLEAN-LAST-NOT: -disable-free
+// NOCLEAN-LAST-SAME: {{.*}}2.cpp
+// NOCLEAN-LAST: cc1
+// NOCLEAN-LAST-SAME: -disable-free
+// NOCLEAN-LAST-SAME: {{.*}}3.cpp
+
+// Four jobs, no invocation will skip clean up because the last job is linking.
+// RUN: %clang -fintegrated-cc1 %t1.cpp %t2.cpp %t3.cpp -### 2>&1 | FileCheck %s --check-prefix=ALL
+// ALL-NOT: -disable-free
+
+// In no-integrated-cc1 mode, no cleanup is needed, because we're running the
+// CC1 tool in a secondary process and the process' heap will be released
+// anyway when the process ends.
+// RUN: %clang -fno-integrated-cc1 -c %t1.cpp -### 2>&1 | FileCheck %s --check-prefix=CLEAN
+
+// RUN: %clang -fno-integrated-cc1 -c %t1.cpp %t2.cpp %t3.cpp -### 2>&1 | FileCheck %s --check-prefix=CLEANALL
+// CLEANALL: cc1
+// CLEANALL-SAME: -disable-free
+// CLEANALL-SAME: {{.*}}1.cpp
+// CLEANALL: cc1
+// CLEANALL-SAME: -disable-free
+// CLEANALL-SAME: {{.*}}2.cpp
+// CLEANALL: cc1
+// CLEANALL-SAME: -disable-free
+// CLEANALL-SAME: {{.*}}3.cpp
+
+// RUN: %clang -fno-integrated-cc1 %t1.cpp %t2.cpp %t3.cpp -### 2>&1 | FileCheck %s --check-prefix=CLEANALL

clang/tools/driver/cc1_main.cpp

if (auto profilerOutput =
                                     /*RemoveFileOnSignal=*/false, "",
                                     /*Extension=*/"json",
                                     /*useTemporary=*/false)) {
 
       llvm::timeTraceProfilerWrite(*profilerOutput);
       // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
       profilerOutput->flush();
       llvm::timeTraceProfilerCleanup();
+      Clang->clearOutputFiles(false);
     }
   }
 
   // Our error handler depends on the Diagnostics object, which we're
   // potentially about to delete. Uninstall the handler now so that any
   // later errors use the default handling behavior instead.
   llvm::remove_fatal_error_handler();
 
   // When running with -disable-free, don't do any destruction or shutdown.
   if (Clang->getFrontendOpts().DisableFree) {
     llvm::BuryPointer(std::move(Clang));
     return !Success;
   }
 
   return !Success;
 }