Skip to content

[lldb] Fix a crash when two diagnostics are on the same column or in … #112451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 16, 2024
Merged

[lldb] Fix a crash when two diagnostics are on the same column or in … #112451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 25 additions & 7 deletions lldb/source/Utility/DiagnosticsRendering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,7 @@ void RenderDiagnosticDetails(Stream &stream,
spacer = "";
}

// Print a line with caret indicator(s) below the lldb prompt + command.
const size_t padding = *offset_in_command;
stream << std::string(padding, ' ');

size_t offset = 1;
// Partition the diagnostics.
std::vector<DiagnosticDetail> remaining_details, other_details,
hidden_details;
for (const DiagnosticDetail &detail : details) {
Expand All @@ -98,10 +94,31 @@ void RenderDiagnosticDetails(Stream &stream,
continue;
}

auto &loc = *detail.source_location;
remaining_details.push_back(detail);
}

// Sort the diagnostics.
auto sort = [](auto &ds) {
llvm::sort(ds.begin(), ds.end(), [](auto &d1, auto &d2) {
auto l1 = d1.source_location.value_or(DiagnosticDetail::SourceLocation{});
auto l2 = d2.source_location.value_or(DiagnosticDetail::SourceLocation{});
return std::pair(l1.line, l2.column) < std::pair(l1.line, l2.column);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comparison is always false, both sides of the < operator are identical.

Copy link
Collaborator

@slackito slackito Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I found that bug because this commit is causing flakiness in the test. So the bad comparison is producing the expected results, sometimes.

Some more data I gathered poking around to hopefully help debug. If I change the comparator to

  return std::pair(l1.line, l1.column) < std::pair(l2.line, l2.column)

I still get (non-flaky) test failures. I think at least one of these test cases is wrong:

  {
    // Test that diagnostics in reverse order are emitted correctly.
    SourceLocation loc1 = {FileSpec{"a.c"}, 1, 20, 0, false, true};
    SourceLocation loc2 = {FileSpec{"a.c"}, 2, 10, 0, false, true};
    std::string result =
        Render({DiagnosticDetail{loc2, eSeverityError, "X", "X"},
                DiagnosticDetail{loc1, eSeverityError, "Y", "Y"}});
    ASSERT_LT(StringRef(result).find("Y"), StringRef(result).find("X"));
  }
  {
    // Test that diagnostics in reverse order are emitted correctly.
    SourceLocation loc1 = {FileSpec{"a.c"}, 2, 10, 0, false, true};
    SourceLocation loc2 = {FileSpec{"a.c"}, 1, 20, 0, false, true};
    std::string result =
        Render({DiagnosticDetail{loc2, eSeverityError, "X", "X"},
                DiagnosticDetail{loc1, eSeverityError, "Y", "Y"}});
    ASSERT_LT(StringRef(result).find("Y"), StringRef(result).find("X"));
  }

In the first one, we have "X" in loc2 (line 2, col 10), and "Y" in loc1 (line 1, col 20), so they are in reverse order.
In the second one, we have "X" in loc2 (line 1, col 20), and "Y" in loc1 (line 2, col 10), so despite the comment they are not in reverse order if lines are supposed to be compared first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#113220

});
};
sort(remaining_details);
sort(other_details);
sort(hidden_details);

// Print a line with caret indicator(s) below the lldb prompt + command.
const size_t padding = *offset_in_command;
stream << std::string(padding, ' ');
size_t offset = 1;
for (const DiagnosticDetail &detail : remaining_details) {
auto &loc = *detail.source_location;

if (offset > loc.column)
continue;

stream << std::string(loc.column - offset, ' ') << cursor;
for (unsigned i = 0; i + 1 < loc.length; ++i)
stream << underline;
Expand All @@ -121,7 +138,8 @@ void RenderDiagnosticDetails(Stream &stream,
for (auto &remaining_detail :
llvm::ArrayRef(remaining_details).drop_back(1)) {
uint16_t column = remaining_detail.source_location->column;
stream << std::string(column - offset, ' ') << vbar;
if (offset <= column)
stream << std::string(column - offset, ' ') << vbar;
offset = column + 1;
}

Expand Down
37 changes: 35 additions & 2 deletions lldb/unittests/Utility/DiagnosticsRenderingTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,45 @@ std::string Render(std::vector<DiagnosticDetail> details) {
} // namespace

TEST_F(ErrorDisplayTest, RenderStatus) {
DiagnosticDetail::SourceLocation inline_loc;
inline_loc.in_user_input = true;
using SourceLocation = DiagnosticDetail::SourceLocation;
{
SourceLocation inline_loc;
inline_loc.in_user_input = true;
std::string result =
Render({DiagnosticDetail{inline_loc, eSeverityError, "foo", ""}});
ASSERT_TRUE(StringRef(result).contains("error:"));
ASSERT_TRUE(StringRef(result).contains("foo"));
}

{
// Test that diagnostics on the same column can be handled and all
// three errors are diagnosed.
SourceLocation loc1 = {FileSpec{"a.c"}, 13, 11, 0, false, true};
SourceLocation loc2 = {FileSpec{"a.c"}, 13, 13, 0, false, true};
std::string result =
Render({DiagnosticDetail{loc1, eSeverityError, "1", "1"},
DiagnosticDetail{loc1, eSeverityError, "2", "2"},
DiagnosticDetail{loc2, eSeverityError, "3", "3"}});
ASSERT_TRUE(StringRef(result).contains("error: 1"));
ASSERT_TRUE(StringRef(result).contains("error: 2"));
ASSERT_TRUE(StringRef(result).contains("error: 3"));
}
{
// Test that diagnostics in reverse order are emitted correctly.
SourceLocation loc1 = {FileSpec{"a.c"}, 1, 20, 0, false, true};
SourceLocation loc2 = {FileSpec{"a.c"}, 2, 10, 0, false, true};
std::string result =
Render({DiagnosticDetail{loc2, eSeverityError, "X", "X"},
DiagnosticDetail{loc1, eSeverityError, "Y", "Y"}});
ASSERT_LT(StringRef(result).find("Y"), StringRef(result).find("X"));
}
{
// Test that diagnostics in reverse order are emitted correctly.
SourceLocation loc1 = {FileSpec{"a.c"}, 2, 10, 0, false, true};
SourceLocation loc2 = {FileSpec{"a.c"}, 1, 20, 0, false, true};
std::string result =
Render({DiagnosticDetail{loc2, eSeverityError, "X", "X"},
DiagnosticDetail{loc1, eSeverityError, "Y", "Y"}});
ASSERT_LT(StringRef(result).find("Y"), StringRef(result).find("X"));
}
}
Loading