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.

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

Conversation

adrian-prantl
Copy link
Collaborator

…reverse order

The second inner loop (only) was missing the check for offset > column. Also this patch sorts the diagnostics before printing them.

@llvmbot
Copy link
Member

llvmbot commented Oct 15, 2024

@llvm/pr-subscribers-lldb

Author: Adrian Prantl (adrian-prantl)

Changes

…reverse order

The second inner loop (only) was missing the check for offset > column. Also this patch sorts the diagnostics before printing them.


Full diff: https://github.com/llvm/llvm-project/pull/112451.diff

2 Files Affected:

  • (modified) lldb/source/Utility/DiagnosticsRendering.cpp (+25-7)
  • (modified) lldb/unittests/Utility/DiagnosticsRenderingTest.cpp (+26-2)
diff --git a/lldb/source/Utility/DiagnosticsRendering.cpp b/lldb/source/Utility/DiagnosticsRendering.cpp
index 96caf934cc2315..dd8f5dd1700c26 100644
--- a/lldb/source/Utility/DiagnosticsRendering.cpp
+++ b/lldb/source/Utility/DiagnosticsRendering.cpp
@@ -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) {
@@ -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) {
+    std::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);
+    });
+  };
+  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;
@@ -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;
     }
 
diff --git a/lldb/unittests/Utility/DiagnosticsRenderingTest.cpp b/lldb/unittests/Utility/DiagnosticsRenderingTest.cpp
index 2bd80796b8074c..e55b23368f0ea2 100644
--- a/lldb/unittests/Utility/DiagnosticsRenderingTest.cpp
+++ b/lldb/unittests/Utility/DiagnosticsRenderingTest.cpp
@@ -16,12 +16,36 @@ std::string Render(std::vector<DiagnosticDetail> details) {
 } // namespace
 
 TEST_F(ErrorDisplayTest, RenderStatus) {
-  DiagnosticDetail::SourceLocation inline_loc;
-  inline_loc.in_user_input = true;
   {
+    DiagnosticDetail::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"));
   }
+
+  {
+    DiagnosticDetail::SourceLocation loc1 = {FileSpec{"a.c"}, 13,  11, 0,
+                                             false,           true};
+    DiagnosticDetail::SourceLocation loc2 = {FileSpec{"a.c"}, 13,  13, 0,
+                                             false,           true};
+    std::string result =
+        Render({DiagnosticDetail{loc1, eSeverityError, "1", "1"},
+                DiagnosticDetail{loc1, eSeverityError, "2", "3"},
+                DiagnosticDetail{loc2, eSeverityError, "3", "3"}});
+    ASSERT_TRUE(StringRef(result).contains("error: 1"));
+    ASSERT_TRUE(StringRef(result).contains("error: 3"));
+    ASSERT_TRUE(StringRef(result).contains("error: 2"));
+  }
+  {
+    DiagnosticDetail::SourceLocation loc1 = {FileSpec{"a.c"}, 1,   20, 0,
+                                             false,           true};
+    DiagnosticDetail::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"));
+  }
 }

…reverse order

The second inner loop (only) was missing the check for
offset > column. Also this patch sorts the diagnostics before printing them.
Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

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

Thanks!

@adrian-prantl adrian-prantl merged commit 889e6ad into llvm:main Oct 16, 2024
5 of 6 checks passed
adrian-prantl added a commit to adrian-prantl/llvm-project that referenced this pull request Oct 16, 2024
llvm#112451)

…reverse order

The second inner loop (only) was missing the check for offset > column.
Also this patch sorts the diagnostics before printing them.

(cherry picked from commit 889e6ad)
adrian-prantl added a commit to adrian-prantl/llvm-project that referenced this pull request Oct 16, 2024
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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants