From 11f8a943bfafd445f4fbd3e126b0f88f7d039f47 Mon Sep 17 00:00:00 2001
From: David Blaikie <dblaikie@gmail.com>
Date: Mon, 15 Sep 2014 17:30:56 +0000
Subject: [PATCH] Fix memory leak of raw_ostreams in LogDiagnosticPrinter
 handling.

This is another case of conditional ownership (in this case a raw
reference, plus a boolean to indicate whether the referenced object
should be deleted). While it's not ideal, I prefer to make the ownership
explicit with a unique_ptr than using a boolean flag (though it does
make the reference and the unique_ptr redundant in the sense that they
both refer to the same memory). At some point we might write a reusable
conditional ownership pointer (a stateful custom deleter for a unique_ptr
may be appropriate).

Based on a patch from a patch by Anton Yartsev.

llvm-svn: 217791
---
 .../clang/Frontend/LogDiagnosticPrinter.h        |  8 +++++---
 clang/lib/Frontend/CompilerInstance.cpp          | 14 +++++++-------
 clang/lib/Frontend/LogDiagnosticPrinter.cpp      | 16 +++++-----------
 3 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/clang/include/clang/Frontend/LogDiagnosticPrinter.h b/clang/include/clang/Frontend/LogDiagnosticPrinter.h
index d021a4d3ce74..8d60e9bd267c 100644
--- a/clang/include/clang/Frontend/LogDiagnosticPrinter.h
+++ b/clang/include/clang/Frontend/LogDiagnosticPrinter.h
@@ -43,13 +43,16 @@ class LogDiagnosticPrinter : public DiagnosticConsumer {
   void EmitDiagEntry(llvm::raw_ostream &OS,
                      const LogDiagnosticPrinter::DiagEntry &DE);
 
+  // Conditional ownership (when StreamOwner is non-null, it's keeping OS
+  // alive). We might want to replace this with a wrapper for conditional
+  // ownership eventually - it seems to pop up often enough.
   raw_ostream &OS;
+  std::unique_ptr<raw_ostream> StreamOwner;
   const LangOptions *LangOpts;
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
 
   SourceLocation LastWarningLoc;
   FullSourceLoc LastLoc;
-  unsigned OwnsOutputStream : 1;
 
   SmallVector<DiagEntry, 8> Entries;
 
@@ -58,8 +61,7 @@ class LogDiagnosticPrinter : public DiagnosticConsumer {
 
 public:
   LogDiagnosticPrinter(raw_ostream &OS, DiagnosticOptions *Diags,
-                       bool OwnsOutputStream = false);
-  virtual ~LogDiagnosticPrinter();
+                       std::unique_ptr<raw_ostream> StreamOwner);
 
   void setDwarfDebugFlags(StringRef Value) {
     DwarfDebugFlags = Value;
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index ec4817d61faf..395d85f77400 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -135,27 +135,27 @@ static void SetUpDiagnosticLog(DiagnosticOptions *DiagOpts,
                                const CodeGenOptions *CodeGenOpts,
                                DiagnosticsEngine &Diags) {
   std::error_code EC;
-  bool OwnsStream = false;
+  std::unique_ptr<raw_ostream> StreamOwner;
   raw_ostream *OS = &llvm::errs();
   if (DiagOpts->DiagnosticLogFile != "-") {
     // Create the output stream.
-    llvm::raw_fd_ostream *FileOS(new llvm::raw_fd_ostream(
+    auto FileOS = llvm::make_unique<llvm::raw_fd_ostream>(
         DiagOpts->DiagnosticLogFile, EC,
-        llvm::sys::fs::F_Append | llvm::sys::fs::F_Text));
+        llvm::sys::fs::F_Append | llvm::sys::fs::F_Text);
     if (EC) {
       Diags.Report(diag::warn_fe_cc_log_diagnostics_failure)
           << DiagOpts->DiagnosticLogFile << EC.message();
     } else {
       FileOS->SetUnbuffered();
       FileOS->SetUseAtomicWrites(true);
-      OS = FileOS;
-      OwnsStream = true;
+      OS = FileOS.get();
+      StreamOwner = std::move(FileOS);
     }
   }
 
   // Chain in the diagnostic client which will log the diagnostics.
-  LogDiagnosticPrinter *Logger = new LogDiagnosticPrinter(*OS, DiagOpts,
-                                                          OwnsStream);
+  LogDiagnosticPrinter *Logger =
+      new LogDiagnosticPrinter(*OS, DiagOpts, std::move(StreamOwner));
   if (CodeGenOpts)
     Logger->setDwarfDebugFlags(CodeGenOpts->DwarfDebugFlags);
   Diags.setClient(new ChainedDiagnosticConsumer(Diags.takeClient(), Logger));
diff --git a/clang/lib/Frontend/LogDiagnosticPrinter.cpp b/clang/lib/Frontend/LogDiagnosticPrinter.cpp
index 19539e0a5eb2..c2dcd1be313b 100644
--- a/clang/lib/Frontend/LogDiagnosticPrinter.cpp
+++ b/clang/lib/Frontend/LogDiagnosticPrinter.cpp
@@ -18,17 +18,11 @@
 using namespace clang;
 using namespace markup;
 
-LogDiagnosticPrinter::LogDiagnosticPrinter(raw_ostream &os,
-                                           DiagnosticOptions *diags,
-                                           bool _OwnsOutputStream)
-  : OS(os), LangOpts(nullptr), DiagOpts(diags),
-    OwnsOutputStream(_OwnsOutputStream) {
-}
-
-LogDiagnosticPrinter::~LogDiagnosticPrinter() {
-  if (OwnsOutputStream)
-    delete &OS;
-}
+LogDiagnosticPrinter::LogDiagnosticPrinter(
+    raw_ostream &os, DiagnosticOptions *diags,
+    std::unique_ptr<raw_ostream> StreamOwner)
+    : OS(os), StreamOwner(std::move(StreamOwner)), LangOpts(nullptr),
+      DiagOpts(diags) {}
 
 static StringRef getLevelName(DiagnosticsEngine::Level Level) {
   switch (Level) {