diagnostic: Use shared_ptr for owned Source::Files

It's too easy to copy diagnostics around and lose track of Source::File
ownership. Ideally we'd place the shared_ptr on the Source, but Sources
are copied _very_ frequently, and we'd lose a huge amount of
performance. Typically, Source::Files are owned externally. The only
time we really need to hold a shared_ptr to these is when a Source::File
is generated by an ICE, as the File points to the C++ source file that
raised the error.

Bug: chromium:1292829
Bug: tint:1383
Change-Id: I2706de8775bc3366115865b5a94785c0b2fefaae
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/78782
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>

src/debug.cc

@@ -14,6 +14,8 @@
 
 #include "src/debug.h"
 
+#include <memory>
+
 namespace tint {
 namespace {
 
@@ -32,9 +34,9 @@ InternalCompilerError::InternalCompilerError(const char* file,
     : file_(file), line_(line), system_(system), diagnostics_(diagnostics) {}
 
 InternalCompilerError::~InternalCompilerError() {
-  Source source{Source::Range{{line_}}, new Source::File{file_, ""}};
+  auto file = std::make_shared<Source::File>(file_, "");
-  diagnostics_.own_file(source.file);
+  Source source{Source::Range{{line_}}, file.get()};
-  diagnostics_.add_ice(system_, msg_.str(), source);
+  diagnostics_.add_ice(system_, msg_.str(), source, std::move(file));
 
   if (ice_reporter) {
     ice_reporter(diagnostics_);

src/diagnostic/diagnostic.cc

@@ -21,39 +21,20 @@
 namespace tint {
 namespace diag {
 
+Diagnostic::Diagnostic() = default;
+Diagnostic::Diagnostic(const Diagnostic&) = default;
+Diagnostic::~Diagnostic() = default;
+Diagnostic& Diagnostic::operator=(const Diagnostic&) = default;
+
 List::List() = default;
 List::List(std::initializer_list<Diagnostic> list) : entries_(list) {}
-List::List(const List& rhs) {
+List::List(const List& rhs) = default;
-  *this = rhs;
-}
 
 List::List(List&& rhs) = default;
 
 List::~List() = default;
 
-List& List::operator=(const List& rhs) {
+List& List::operator=(const List& rhs) = default;
-  // Create copies of any of owned files, maintaining a map of rhs-file to
-  // new-file.
-  std::unordered_map<const Source::File*, const Source::File*> files;
-  owned_files_.reserve(rhs.owned_files_.size());
-  files.reserve(rhs.owned_files_.size());
-  for (auto& rhs_file : rhs.owned_files_) {
-    auto file = std::make_unique<Source::File>(*rhs_file);
-    files.emplace(rhs_file.get(), file.get());
-    owned_files_.emplace_back(std::move(file));
-  }
-
-  // Copy the diagnostic entries, then fix up pointers to the file copies.
-  entries_ = rhs.entries_;
-  for (auto& entry : entries_) {
-    if (auto it = files.find(entry.source.file); it != files.end()) {
-      entry.source.file = it->second;
-    }
-  }
-
-  error_count_ = rhs.error_count_;
-  return *this;
-}
 
 List& List::operator=(List&& rhs) = default;
 

src/diagnostic/diagnostic.h

@@ -55,6 +55,17 @@ enum class System {
 /// message.
 class Diagnostic {
  public:
+  /// Constructor
+  Diagnostic();
+  /// Copy constructor
+  Diagnostic(const Diagnostic&);
+  /// Destructor
+  ~Diagnostic();
+
+  /// Copy assignment operator
+  /// @return this diagnostic
+  Diagnostic& operator=(const Diagnostic&);
+
   /// severity is the severity of the diagnostic message.
   Severity severity = Severity::Error;
   /// source is the location of the diagnostic.
@@ -66,6 +77,10 @@ class Diagnostic {
   /// code is the error code, for example a validation error might have the code
   /// `"v-0001"`.
   const char* code = nullptr;
+  /// A shared pointer to a Source::File. Only used if the diagnostic Source
+  /// points to a file that was created specifically for this diagnostic
+  /// (usually an ICE).
+  std::shared_ptr<Source::File> owned_file = nullptr;
 };
 
 /// List is a container of Diagnostic messages.
@@ -197,24 +212,20 @@ class List {
   /// @param system the system raising the error message
   /// @param err_msg the error message
   /// @param source the source of the internal compiler error
+  /// @param file the Source::File owned by this diagnostic
   void add_ice(System system,
                const std::string& err_msg,
-               const Source& source) {
+               const Source& source,
+               std::shared_ptr<Source::File> file) {
     diag::Diagnostic ice{};
     ice.severity = diag::Severity::InternalCompilerError;
     ice.system = system;
     ice.source = source;
     ice.message = err_msg;
+    ice.owned_file = std::move(file);
     add(std::move(ice));
   }
 
-  /// Adds the file to the list of files owned by this diagnostic list.
-  /// When this list is destructed, all the owned files will be deleted.
-  /// @param file the file that this List should own
-  void own_file(const Source::File* file) {
-    owned_files_.emplace_back(std::unique_ptr<const Source::File>(file));
-  }
-
   /// @returns true iff the diagnostic list contains errors diagnostics (or of
   /// higher severity).
   bool contains_errors() const { return error_count_ > 0; }
@@ -232,7 +243,6 @@ class List {
 
  private:
   std::vector<Diagnostic> entries_;
-  std::vector<std::unique_ptr<const Source::File>> owned_files_;
   size_t error_count_ = 0;
 };
 

src/diagnostic/diagnostic_test.cc

@@ -21,43 +21,20 @@ namespace tint {
 namespace diag {
 namespace {
 
-TEST(DiagListTest, UnownedFilesNotCopied) {
+TEST(DiagListTest, OwnedFilesShared) {
-  Source::File file{"path", "content"};
+  auto file = std::make_shared<Source::File>("path", "content");
 
   diag::List list_a, list_b;
   {
     diag::Diagnostic diag{};
-    diag.source = Source{Source::Range{{0, 0}}, &file};
+    diag.source = Source{Source::Range{{0, 0}}, file.get()};
     list_a.add(std::move(diag));
   }
 
   list_b = list_a;
 
   ASSERT_EQ(list_b.count(), list_a.count());
-  EXPECT_EQ(list_b.begin()->source.file, &file);
+  EXPECT_EQ(list_b.begin()->source.file, file.get());
-}
-
-TEST(DiagListTest, OwnedFilesCopied) {
-  auto* file = new Source::File{"path", "content"};
-
-  diag::List list_a, list_b;
-  {
-    diag::Diagnostic diag{};
-    diag.source = Source{Source::Range{{0, 0}}, file};
-    list_a.add(std::move(diag));
-    list_a.own_file(file);
-  }
-
-  list_b = list_a;
-
-  ASSERT_EQ(list_b.count(), list_a.count());
-  EXPECT_NE(list_b.begin()->source.file, file);
-  ASSERT_NE(list_b.begin()->source.file, nullptr);
-  EXPECT_EQ(list_b.begin()->source.file->path, file->path);
-  EXPECT_EQ(list_b.begin()->source.file->content.data, file->content.data);
-  EXPECT_EQ(list_b.begin()->source.file->content.data_view,
-            file->content.data_view);
-  EXPECT_EQ(list_b.begin()->source.file->content.lines, file->content.lines);
 }
 
 }  // namespace

src/diagnostic/formatter_test.cc

@@ -14,6 +14,8 @@
 
 #include "src/diagnostic/formatter.h"
 
+#include <utility>
+
 #include "gtest/gtest.h"
 #include "src/diagnostic/diagnostic.h"
 
@@ -21,6 +23,20 @@ namespace tint {
 namespace diag {
 namespace {
 
+Diagnostic Diag(Severity severity,
+                Source source,
+                std::string message,
+                System system,
+                const char* code = nullptr) {
+  Diagnostic d;
+  d.severity = severity;
+  d.source = source;
+  d.message = std::move(message);
+  d.system = system;
+  d.code = code;
+  return d;
+}
+
 constexpr const char* content =  // Note: words are tab-delimited
     R"(the	cat	says	meow
 the	dog	says	woof
@@ -31,21 +47,28 @@ the	snail	says	???
 class DiagFormatterTest : public testing::Test {
  public:
   Source::File file{"file.name", content};
-  Diagnostic diag_note{Severity::Note,
+  Diagnostic diag_note =
-                       Source{Source::Range{Source::Location{1, 14}}, &file},
+      Diag(Severity::Note,
-                       "purr", System::Test};
+           Source{Source::Range{Source::Location{1, 14}}, &file},
-  Diagnostic diag_warn{Severity::Warning,
+           "purr",
-                       Source{Source::Range{{2, 14}, {2, 18}}, &file}, "grrr",
+           System::Test);
-                       System::Test};
+  Diagnostic diag_warn = Diag(Severity::Warning,
-  Diagnostic diag_err{Severity::Error,
+                              Source{Source::Range{{2, 14}, {2, 18}}, &file},
-                      Source{Source::Range{{3, 16}, {3, 21}}, &file}, "hiss",
+                              "grrr",
-                      System::Test, "abc123"};
+                              System::Test);
-  Diagnostic diag_ice{Severity::InternalCompilerError,
+  Diagnostic diag_err = Diag(Severity::Error,
-                      Source{Source::Range{{4, 16}, {4, 19}}, &file},
+                             Source{Source::Range{{3, 16}, {3, 21}}, &file},
-                      "unreachable", System::Test};
+                             "hiss",
-  Diagnostic diag_fatal{Severity::Fatal,
+                             System::Test,
-                        Source{Source::Range{{4, 16}, {4, 19}}, &file},
+                             "abc123");
-                        "nothing", System::Test};
+  Diagnostic diag_ice = Diag(Severity::InternalCompilerError,
+                             Source{Source::Range{{4, 16}, {4, 19}}, &file},
+                             "unreachable",
+                             System::Test);
+  Diagnostic diag_fatal = Diag(Severity::Fatal,
+                               Source{Source::Range{{4, 16}, {4, 19}}, &file},
+                               "nothing",
+                               System::Test);
 };
 
 TEST_F(DiagFormatterTest, Simple) {
@@ -69,7 +92,7 @@ TEST_F(DiagFormatterTest, SimpleNewlineAtEnd) {
 
 TEST_F(DiagFormatterTest, SimpleNoSource) {
   Formatter fmt{{false, false, false, false}};
-  Diagnostic diag{Severity::Note, Source{}, "no source!", System::Test};
+  auto diag = Diag(Severity::Note, Source{}, "no source!", System::Test);
   auto got = fmt.format(List{diag});
   auto* expect = "no source!";
   ASSERT_EQ(expect, got);
@@ -130,9 +153,9 @@ the  snake  says  quack
 }
 
 TEST_F(DiagFormatterTest, BasicWithMultiLine) {
-  Diagnostic multiline{Severity::Warning,
+  auto multiline =
-                       Source{Source::Range{{2, 9}, {4, 15}}, &file},
+      Diag(Severity::Warning, Source{Source::Range{{2, 9}, {4, 15}}, &file},
-                       "multiline", System::Test};
+           "multiline", System::Test);
   Formatter fmt{{false, false, true, false}};
   auto got = fmt.format(List{multiline});
   auto* expect = R"(2:9: multiline
@@ -165,9 +188,9 @@ the    snake    says    quack
 }
 
 TEST_F(DiagFormatterTest, BasicWithMultiLineTab4) {
-  Diagnostic multiline{Severity::Warning,
+  auto multiline =
-                       Source{Source::Range{{2, 9}, {4, 15}}, &file},
+      Diag(Severity::Warning, Source{Source::Range{{2, 9}, {4, 15}}, &file},
-                       "multiline", System::Test};
+           "multiline", System::Test);
   Formatter fmt{{false, false, true, false, 4u}};
   auto got = fmt.format(List{multiline});
   auto* expect = R"(2:9: multiline