-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[lldb] add a marker before skipped frames #167550
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
base: main
Are you sure you want to change the base?
[lldb] add a marker before skipped frames #167550
Conversation
|
@llvm/pr-subscribers-lldb Author: Charles Zablit (charles-zablit) ChangesThis patch adds a marker to make skipped frames more explicit. Skipped frames can be confusing for some users, who see that the indexes of the frames in a backtrace are not contiguous. This patch aims to lessen the confusion by adding a delimiter for the first and last non skipped frame, i.e the boundaries. IDE's like Xcode and VSCode represent those in the UI by having the skipped frames either greyed out or collapsed. It's not possible to do this in the CLI, therefore, this patch makes use of 2 unicode characters to mark the beginning and end of the skipped frames range. In the example below, frame #2 is skipped, and therefore, frame #1 is the first skipped frame of the range while frame #3 is the last non skipped frame: Full diff: https://github.com/llvm/llvm-project/pull/167550.diff 4 Files Affected:
diff --git a/lldb/include/lldb/Target/StackFrame.h b/lldb/include/lldb/Target/StackFrame.h
index cdbe8ae3c6779..c3e3c945e114c 100644
--- a/lldb/include/lldb/Target/StackFrame.h
+++ b/lldb/include/lldb/Target/StackFrame.h
@@ -335,11 +335,16 @@ class StackFrame : public ExecutionContextScope,
/// \param[in] frame_marker
/// Optional string that will be prepended to the frame output description.
///
+ /// \param[in] skipped_frame_marker
+ /// Optional string that will be prepended to the first or last non skipped
+ /// frame output description.
+ ///
/// \return
/// \b true if and only if dumping with the given \p format worked.
bool DumpUsingFormat(Stream &strm,
const lldb_private::FormatEntity::Entry *format,
- llvm::StringRef frame_marker = {});
+ llvm::StringRef frame_marker = {},
+ llvm::StringRef skipped_frame_marker = {});
/// Print a description for this frame using the frame-format formatter
/// settings. If the current frame-format settings are invalid, then the
@@ -353,8 +358,13 @@ class StackFrame : public ExecutionContextScope,
///
/// \param [in] frame_marker
/// Optional string that will be prepended to the frame output description.
+ ///
+ /// \param[in] skipped_frame_marker
+ /// Optional string that will be prepended to the first or last non skipped
+ /// frame output description.
void DumpUsingSettingsFormat(Stream *strm, bool show_unique = false,
- const char *frame_marker = nullptr);
+ const char *frame_marker = nullptr,
+ const std::wstring skipped_frame_marker = L"");
/// Print a description for this frame using a default format.
///
@@ -387,10 +397,15 @@ class StackFrame : public ExecutionContextScope,
/// \param[in] frame_marker
/// Passed to DumpUsingSettingsFormat() for the frame info printing.
///
+ ///
+ /// \param[in] skipped_frame_marker
+ /// Optional string that will be prepended to the first or last non skipped
+ /// frame output description.
/// \return
/// Returns true if successful.
bool GetStatus(Stream &strm, bool show_frame_info, bool show_source,
- bool show_unique = false, const char *frame_marker = nullptr);
+ bool show_unique = false, const char *frame_marker = nullptr,
+ const std::wstring skipped_frame_marker = L"");
/// Query whether this frame is a concrete frame on the call stack, or if it
/// is an inlined frame derived from the debug information and presented by
diff --git a/lldb/include/lldb/Target/StackFrameList.h b/lldb/include/lldb/Target/StackFrameList.h
index ea9aab86b8ea1..122c256bd61d3 100644
--- a/lldb/include/lldb/Target/StackFrameList.h
+++ b/lldb/include/lldb/Target/StackFrameList.h
@@ -49,6 +49,14 @@ class StackFrameList {
/// Resets the selected frame index of this object.
void ClearSelectedFrameIndex();
+ /// Return \code true if the next frame is hidden. False otherwise or if it's
+ /// the last frame.
+ bool IsNextFrameHidden(lldb_private::StackFrame &frame);
+
+ /// Return \code true if the previous frame is hidden. False otherwise or if
+ /// it's the first frame.
+ bool IsPreviousFrameHidden(lldb_private::StackFrame &frame);
+
/// Get the currently selected frame index.
/// We should only call SelectMostRelevantFrame if (a) the user hasn't already
/// selected a frame, and (b) if this really is a user facing
diff --git a/lldb/source/Target/StackFrame.cpp b/lldb/source/Target/StackFrame.cpp
index 2ed58c5331df4..03c595cd49656 100644
--- a/lldb/source/Target/StackFrame.cpp
+++ b/lldb/source/Target/StackFrame.cpp
@@ -37,6 +37,7 @@
#include "lldb/ValueObject/ValueObjectConstResult.h"
#include "lldb/ValueObject/ValueObjectMemory.h"
#include "lldb/ValueObject/ValueObjectVariable.h"
+#include "llvm/Support/ConvertUTF.h"
#include "lldb/lldb-enumerations.h"
@@ -1920,11 +1921,13 @@ void StackFrame::CalculateExecutionContext(ExecutionContext &exe_ctx) {
bool StackFrame::DumpUsingFormat(Stream &strm,
const FormatEntity::Entry *format,
- llvm::StringRef frame_marker) {
+ llvm::StringRef frame_marker,
+ llvm::StringRef skipped_frame_marker) {
GetSymbolContext(eSymbolContextEverything);
ExecutionContext exe_ctx(shared_from_this());
StreamString s;
s.PutCString(frame_marker);
+ s.PutCString(skipped_frame_marker);
if (format && FormatEntity::Format(*format, s, &m_sc, &exe_ctx, nullptr,
nullptr, false, false)) {
@@ -1934,8 +1937,9 @@ bool StackFrame::DumpUsingFormat(Stream &strm,
return false;
}
-void StackFrame::DumpUsingSettingsFormat(Stream *strm, bool show_unique,
- const char *frame_marker) {
+void StackFrame::DumpUsingSettingsFormat(
+ Stream *strm, bool show_unique, const char *frame_marker,
+ const std::wstring skipped_frame_marker) {
if (strm == nullptr)
return;
@@ -1953,7 +1957,11 @@ void StackFrame::DumpUsingSettingsFormat(Stream *strm, bool show_unique,
frame_format = &format_entry;
}
}
- if (!DumpUsingFormat(*strm, frame_format, frame_marker)) {
+
+ std::string skipped_frame_delimiter_utf8;
+ llvm::convertWideToUTF8(skipped_frame_marker, skipped_frame_delimiter_utf8);
+ if (!DumpUsingFormat(*strm, frame_format, frame_marker,
+ skipped_frame_delimiter_utf8)) {
Dump(strm, true, false);
strm->EOL();
}
@@ -2034,10 +2042,12 @@ bool StackFrame::HasCachedData() const {
}
bool StackFrame::GetStatus(Stream &strm, bool show_frame_info, bool show_source,
- bool show_unique, const char *frame_marker) {
+ bool show_unique, const char *frame_marker,
+ const std::wstring skipped_frame_marker) {
if (show_frame_info) {
strm.Indent();
- DumpUsingSettingsFormat(&strm, show_unique, frame_marker);
+ DumpUsingSettingsFormat(&strm, show_unique, frame_marker,
+ skipped_frame_marker);
}
if (show_source) {
diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp
index ccf874fc03ebd..80f77e2bcee16 100644
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -879,6 +879,24 @@ StackFrameList::GetStackFrameSPForStackFramePtr(StackFrame *stack_frame_ptr) {
return ret_sp;
}
+bool StackFrameList::IsNextFrameHidden(lldb_private::StackFrame &frame) {
+ uint32_t frame_idx = frame.GetFrameIndex();
+ StackFrameSP frame_sp = GetFrameAtIndex(frame_idx + 1);
+ if (!frame_sp)
+ return false;
+ return frame_sp->IsHidden();
+}
+
+bool StackFrameList::IsPreviousFrameHidden(lldb_private::StackFrame &frame) {
+ uint32_t frame_idx = frame.GetFrameIndex();
+ if (frame_idx == 0)
+ return false;
+ StackFrameSP frame_sp = GetFrameAtIndex(frame_idx - 1);
+ if (!frame_sp)
+ return false;
+ return frame_sp->IsHidden();
+}
+
size_t StackFrameList::GetStatus(Stream &strm, uint32_t first_frame,
uint32_t num_frames, bool show_frame_info,
uint32_t num_frames_with_source,
@@ -920,6 +938,11 @@ size_t StackFrameList::GetStatus(Stream &strm, uint32_t first_frame,
else
marker = unselected_marker;
}
+ std::wstring skipped_frame_marker = L" ";
+ if (IsPreviousFrameHidden(*frame_sp))
+ skipped_frame_marker = L"﹈";
+ else if (IsNextFrameHidden(*frame_sp))
+ skipped_frame_marker = L"﹇";
// Hide uninteresting frames unless it's the selected frame.
if (!show_hidden && frame_sp != selected_frame_sp && frame_sp->IsHidden())
@@ -933,10 +956,9 @@ size_t StackFrameList::GetStatus(Stream &strm, uint32_t first_frame,
m_thread.GetID(), num_frames_displayed))
break;
-
if (!frame_sp->GetStatus(strm, show_frame_info,
num_frames_with_source > (first_frame - frame_idx),
- show_unique, marker))
+ show_unique, marker, skipped_frame_marker))
break;
++num_frames_displayed;
}
|
|
I think this patch still has room for improvements (I should also add some tests). I'm curious about the thoughts people might have about those brackets? |
|
Brackets for indicating elided stuff has precedent in other areas, so that seems pretty clear to me. It's enough to tell you "this is intentional" without being too eye-catching. Seems good to me. |
Thanks! I will try to reformat this a bit so that we don't have this wide space between the |
|
I'm fine with the brackets. Another alternative could be https://www.compart.com/en/unicode/U+FE4D but maybe that ends up being too subtle? |
Could work, I will post a new screenshot 👍 |
| /// Return \code true if the next frame is hidden. False otherwise or if it's | ||
| /// the last frame. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Return \code true if the next frame is hidden. False otherwise or if it's | |
| /// the last frame. | |
| /// Return \code true if the next frame is hidden. |
I'd say it's implied that if we're at the first/last frame, then querying anything about the previous/next frame is not going to be valid. Not sure we need to document that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks!
lldb/source/Target/StackFrame.cpp
Outdated
| const char *frame_marker) { | ||
| void StackFrame::DumpUsingSettingsFormat( | ||
| Stream *strm, bool show_unique, const char *frame_marker, | ||
| const std::wstring skipped_frame_marker) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const std::wstring skipped_frame_marker) { | |
| const std::wstring &skipped_frame_marker) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks!
lldb/source/Target/StackFrame.cpp
Outdated
| @@ -2034,10 +2042,12 @@ bool StackFrame::HasCachedData() const { | |||
| } | |||
|
|
|||
| bool StackFrame::GetStatus(Stream &strm, bool show_frame_info, bool show_source, | |||
| bool show_unique, const char *frame_marker) { | |||
| bool show_unique, const char *frame_marker, | |||
| const std::wstring skipped_frame_marker) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const std::wstring skipped_frame_marker) { | |
| const std::wstring &skipped_frame_marker) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks!
| @@ -920,6 +938,11 @@ size_t StackFrameList::GetStatus(Stream &strm, uint32_t first_frame, | |||
| else | |||
| marker = unselected_marker; | |||
| } | |||
| std::wstring skipped_frame_marker = L" "; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remind me why we need wstring for this? Could this just be a utf-8 std::string from the beginning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No special reason, I pushed the conversion further up the stack in 2c674013262b.
|
I have updated the description of the PR to show the new appearance. |
🐧 Linux x64 Test Results
|
2c67401 to
78385d4
Compare
adrian-prantl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one minor comment inside
| // Host so it can be customized for a specific platform. | ||
| llvm::StringRef cursor, underline, vbar, joint, hbar, spacer; | ||
| if (stream.AsRawOstream().colors_enabled()) { | ||
| if (TerminalSupportsUnicode()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is long overdue!
|
|
||
| void StackFrame::DumpUsingSettingsFormat(Stream *strm, bool show_unique, | ||
| const char *frame_marker) { | ||
| const std::string &frame_marker) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're passing this into a function that takes a StringRef — why not make it a StringRef, too?
| /// The value is cached after the first computation. | ||
| /// | ||
| /// On POSIX systems, we check if the LANG environment variable contains the | ||
| /// substring "UTF-8"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's so much better than checking for color!
| /// Optional string that will be prepended to the frame output description. | ||
| void DumpUsingSettingsFormat(Stream *strm, bool show_unique = false, | ||
| const char *frame_marker = nullptr); | ||
| const std::string &frame_marker = ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringRef?
| /// | ||
| /// On Windows, we check that we are running from the Windows Terminal | ||
| /// application. | ||
| bool TerminalSupportsUnicode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move this into Host/Terminal.h?
| bool TerminalSupportsUnicode() { | ||
| static std::optional<bool> result; | ||
| if (result) | ||
| return result.value(); | ||
| #ifndef _WIN32 | ||
| if (const char *lang_var = std::getenv("LANG")) | ||
| result = std::string(lang_var).find("UTF-8"); | ||
| else | ||
| result = false; | ||
| #else | ||
| result = std::getenv("WT_SESSION") != nullptr; | ||
| #endif | ||
| return result.value(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be correct for Xcode and DAP?
| return result.value(); | ||
| #ifndef _WIN32 | ||
| if (const char *lang_var = std::getenv("LANG")) | ||
| result = std::string(lang_var).find("UTF-8"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| result = std::string(lang_var).find("UTF-8"); | |
| result = llvm::StringRef(lang_var).find("UTF-8"); |
| if (frame_sp == selected_frame_sp) { | ||
| return TerminalSupportsUnicode() ? L" * " : L"* "; | ||
| } else if (!TerminalSupportsUnicode()) { | ||
| return L" "; | ||
| } else if (IsPreviousFrameHidden(*frame_sp)) { | ||
| return L" ﹉"; | ||
| } else if (IsNextFrameHidden(*frame_sp)) { | ||
| return L" ﹍"; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No braces
|
Sorry, all my comments should have gone in the other PR :( |
This patch adds a marker to make skipped frames more explicit.
Skipped frames can be confusing for some users, who see that the indexes of the frames in a backtrace are not contiguous. This patch aims to lessen the confusion by adding a delimiter for the first and last non skipped frame, i.e the boundaries.
IDE's like Xcode and VSCode represent those in the UI by having the skipped frames either greyed out or collapsed.
It's not possible to do this in the CLI, therefore, this patch makes use of 2 unicode characters to mark the beginning and end of the skipped frames range.
This patch depends on:
Examples
In the example below, frame

#2to#7are is skipped, and therefore, frame#1is the first non skipped frame of the range while frame#8is the last non skipped frame:If the selected frame is one of the 2 boundary frames, we replace the delimiter character with the select character (
*).