Skip to content

Commit

Permalink
diagnostic: Use shared_ptr for owned Source::Files
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Kokoro: Kokoro <[email protected]>
Commit-Queue: Ben Clayton <[email protected]>
  • Loading branch information
@ben-clayton
ben-clayton authored and Tint LUCI CQ committed Feb 1, 2022
1 parent e3d4197 commit e1159c7
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 87 deletions.
8 changes: 5 additions & 3 deletions src/debug.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

#include "src/debug.h"

#include <memory>

namespace tint {
namespace {

Expand All @@ -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_, ""}};
diagnostics_.own_file(source.file);
diagnostics_.add_ice(system_, msg_.str(), source);
auto file = std::make_shared<Source::File>(file_, "");
Source source{Source::Range{{line_}}, file.get()};
diagnostics_.add_ice(system_, msg_.str(), source, std::move(file));

if (ice_reporter) {
ice_reporter(diagnostics_);
Expand Down
33 changes: 7 additions & 26 deletions src/diagnostic/diagnostic.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
*this = rhs;
}
List::List(const List& rhs) = default;

List::List(List&& rhs) = default;

List::~List() = default;

List& List::operator=(const List& rhs) {
// 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=(const List& rhs) = default;

List& List::operator=(List&& rhs) = default;

Expand Down
28 changes: 19 additions & 9 deletions src/diagnostic/diagnostic.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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; }
Expand All @@ -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;
};

Expand Down
31 changes: 4 additions & 27 deletions src/diagnostic/diagnostic_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,43 +21,20 @@ namespace tint {
namespace diag {
namespace {

TEST(DiagListTest, UnownedFilesNotCopied) {
Source::File file{"path", "content"};
TEST(DiagListTest, OwnedFilesShared) {
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);
}

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);
EXPECT_EQ(list_b.begin()->source.file, file.get());
}

} // namespace
Expand Down
67 changes: 45 additions & 22 deletions src/diagnostic/formatter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,29 @@

#include "src/diagnostic/formatter.h"

#include <utility>

#include "gtest/gtest.h"
#include "src/diagnostic/diagnostic.h"

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
Expand All @@ -31,21 +47,28 @@ the snail says ???
class DiagFormatterTest : public testing::Test {
public:
Source::File file{"file.name", content};
Diagnostic diag_note{Severity::Note,
Source{Source::Range{Source::Location{1, 14}}, &file},
"purr", System::Test};
Diagnostic diag_warn{Severity::Warning,
Source{Source::Range{{2, 14}, {2, 18}}, &file}, "grrr",
System::Test};
Diagnostic diag_err{Severity::Error,
Source{Source::Range{{3, 16}, {3, 21}}, &file}, "hiss",
System::Test, "abc123"};
Diagnostic diag_ice{Severity::InternalCompilerError,
Source{Source::Range{{4, 16}, {4, 19}}, &file},
"unreachable", System::Test};
Diagnostic diag_fatal{Severity::Fatal,
Source{Source::Range{{4, 16}, {4, 19}}, &file},
"nothing", System::Test};
Diagnostic diag_note =
Diag(Severity::Note,
Source{Source::Range{Source::Location{1, 14}}, &file},
"purr",
System::Test);
Diagnostic diag_warn = Diag(Severity::Warning,
Source{Source::Range{{2, 14}, {2, 18}}, &file},
"grrr",
System::Test);
Diagnostic diag_err = Diag(Severity::Error,
Source{Source::Range{{3, 16}, {3, 21}}, &file},
"hiss",
System::Test,
"abc123");
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) {
Expand All @@ -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);
Expand Down Expand Up @@ -130,9 +153,9 @@ the snake says quack
}

TEST_F(DiagFormatterTest, BasicWithMultiLine) {
Diagnostic multiline{Severity::Warning,
Source{Source::Range{{2, 9}, {4, 15}}, &file},
"multiline", System::Test};
auto multiline =
Diag(Severity::Warning, Source{Source::Range{{2, 9}, {4, 15}}, &file},
"multiline", System::Test);
Formatter fmt{{false, false, true, false}};
auto got = fmt.format(List{multiline});
auto* expect = R"(2:9: multiline
Expand Down Expand Up @@ -165,9 +188,9 @@ the snake says quack
}

TEST_F(DiagFormatterTest, BasicWithMultiLineTab4) {
Diagnostic multiline{Severity::Warning,
Source{Source::Range{{2, 9}, {4, 15}}, &file},
"multiline", System::Test};
auto multiline =
Diag(Severity::Warning, Source{Source::Range{{2, 9}, {4, 15}}, &file},
"multiline", System::Test);
Formatter fmt{{false, false, true, false, 4u}};
auto got = fmt.format(List{multiline});
auto* expect = R"(2:9: multiline
Expand Down

0 comments on commit e1159c7

Please sign in to comment.