Skip to content
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

[lldb-dap] Updating VariableDescription to use GetDescription() as a fallback. #77026

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

ashgti
Copy link
Contributor

@ashgti ashgti commented Jan 4, 2024

When generating a display_value for a variable the current approach calls SBValue::GetValue() and SBValue::GetSummary() to generate a display_value for the SBValue. However, there are cases where both of these return an empty string and the fallback is to print a pointer and type name instead (e.g. FooBarType @ 0x00321).

For swift types, lldb includes a langauge runtime plugin that can generate a description of the object but this is only used with SBValue::GetDescription().

For example:

$ lldb swift-binary
... stop at breakpoint ...
lldb> script
>>> event = lldb.frame.GetValueForVariablePath("event")
>>> print("Value", event.GetValue())
Value None
>>> print("Summary", event.GetSummary())
Summary None
>>> print("Description", event) # __str__ calls SBValue::GetDescription()
Description (main.Event) event = (name = "Greetings", time = 2024-01-04 23:38:06 UTC)

With this change, if GetValue and GetSummary return empty then we try SBValue::GetDescription() as a fallback before using the previous logic of printing <type> @ <addr>.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 4, 2024

@llvm/pr-subscribers-lldb

Author: John Harrison (ashgti)

Changes

When generating a display_value for a variable the current approach calls SBValue::GetValue() and SBValue::GetSummary() to generate a display_value for the SBValue. However, there are cases where both of these return an empty string and the fallback is to print a pointer and type name instead (e.g. FooBarType @ 0x00321).

For swift types, lldb includes a langauge runtime plugin that can generate a description of the object but this is only used with SBValue::GetDescription().

For example:

$ lldb swift-binary
... stop at breakpoint ...
lldb&gt; script
&gt;&gt;&gt; event = lldb.frame.GetValueForVariablePath("event")
&gt;&gt;&gt; print("Value", event.GetValue())
Value None
&gt;&gt;&gt; print("Summary", event.GetSummary())
Summary None
&gt;&gt;&gt; print("Description", event) # __str__ calls SBValue::GetDescription()
Description (main.Event) event = (name = "Greetings", time = 2024-01-04 23:38:06 UTC)

With this change, if GetValue and GetSummary return empty then we try SBValue::GetDescription() as a fallback before using the previous logic of printing &lt;type&gt; @ &lt;addr&gt;.


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

1 Files Affected:

  • (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+7-3)
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index df17ac9d849176..f8ac53ef809e6e 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -1042,10 +1042,14 @@ VariableDescription::VariableDescription(lldb::SBValue v, bool format_hex,
         os_display_value << " " << *effective_summary;
     } else if (effective_summary) {
       os_display_value << *effective_summary;
-
-      // As last resort, we print its type and address if available.
     } else {
-      if (!raw_display_type_name.empty()) {
+      lldb::SBStream description;
+      // Try letting lldb generate a description.
+      if (v.GetDescription(description) && description.GetSize()) {
+        os_display_value << description.GetData();
+
+        // As last resort, we print its type and address if available.
+      } else if (!raw_display_type_name.empty()) {
         os_display_value << raw_display_type_name;
         lldb::addr_t address = v.GetLoadAddress();
         if (address != LLDB_INVALID_ADDRESS)

Copy link
Collaborator

@clayborg clayborg left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Can we add a test?

Copy link

github-actions bot commented Jan 6, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@ashgti
Copy link
Contributor Author

ashgti commented Jan 6, 2024

Looks fine to me. Can we add a test?

Updated the existing tests to check the adjusted format.

One issue I did notice with tests is that the response to an expression will include a response are automatically persisted by SBValue::GetDescription(), so you will see the value name in the response, for example:

// Example swift struct.
struct Event: Identifiable, Comparable {
  let id: String
  let name: String
  let time: DateInterval
  var count = 0
}
> self
(Calendar.Event) $R1 = {
  id = "e1"
  name = "Event 1"
  time = (start = 2024-01-03 17:00:00 UTC, duration = Swift.Double @ 0x0000600001754928)
  count = (_value = 0)
}
> self
(Calendar.Event) $R2 = {
  id = "e1"
  name = "Event 1"
  time = (start = 2024-01-03 17:00:00 UTC, duration = Swift.Double @ 0x0000600001754928)
  count = (_value = 0)
}
> $R2.count + 2
2

Screenshot of how this looks in VS Code:

Screenshot 2024-01-05 at 5 03 43 PM

@ashgti
Copy link
Contributor Author

ashgti commented Jan 9, 2024

cc @walter-erquinigo since this looks like it has similar overlap to the auto generated summeries.

@@ -135,6 +135,18 @@ std::vector<std::string> GetStrings(const llvm::json::Object *obj,
return strs;
}

static std::string GetDescriptionTrimmed(lldb::SBValue &value) {
lldb::SBStream stream;
value.GetDescription(stream);
Copy link
Member

Choose a reason for hiding this comment

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

The method bool SBValue::GetDescription(SBStream &description) returns true always. Could you make it return false if the description couldn't be generated, and also abort early in this function in such case?
I've also noticed that GetDescription dumps "No value" in some cases, which would be better not to display via lldb-dap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated bool SBValue::GetDescription(SBStream &description) to return false for the "No value" case and this helper should return an empty string in that situation fallback back to the <type> @ <pointer> format.

"result": "{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, ...}"
if enableAutoVariableSummaries
else "int[32] @ 0x"
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity, how is this rendered in the variables tab on VSCode? Something I tried to achieve with auto summaries is to use a single line for readability, so I'd like to know how this looks like in the UI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the UI it will look like in the variable view:
Screenshot 2024-01-09 at 11 36 21 AM

Hover in the variable view:
Screenshot 2024-01-09 at 11 36 30 AM

Debug Console:
Screenshot 2024-01-09 at 11 47 52 AM

Debug Console Hover:
Screenshot 2024-01-09 at 11 48 02 AM

Copy link
Member

Choose a reason for hiding this comment

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

Quite an interesting coincidence that it looks nice when the children are expanded.

Copy link
Member

@walter-erquinigo walter-erquinigo left a comment

Choose a reason for hiding this comment

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

just two minor things left, but otherwise LGTM

@@ -135,6 +135,21 @@ std::vector<std::string> GetStrings(const llvm::json::Object *obj,
return strs;
}

static std::string GetDescriptionTrimmed(lldb::SBValue &value) {
Copy link
Member

Choose a reason for hiding this comment

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

add a comment for this function mentioning that this returns an empty string if the description is not available. You can also change the return type to optionalstd::string, which might be cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 140 to 142
if (!value.GetDescription(stream)) {
return "";
}
Copy link
Member

Choose a reason for hiding this comment

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

remove unnecessary braces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the braces (and well simplified this function).

@ashgti ashgti requested a review from clayborg January 9, 2024 23:05
@clayborg
Copy link
Collaborator

Looks fine to me. Can we add a test?

Updated the existing tests to check the adjusted format.

One issue I did notice with tests is that the response to an expression will include a response are automatically persisted by SBValue::GetDescription(), so you will see the value name in the response, for example:

// Example swift struct.
struct Event: Identifiable, Comparable {
  let id: String
  let name: String
  let time: DateInterval
  var count = 0
}
> self
(Calendar.Event) $R1 = {
  id = "e1"
  name = "Event 1"
  time = (start = 2024-01-03 17:00:00 UTC, duration = Swift.Double @ 0x0000600001754928)
  count = (_value = 0)
}
> self
(Calendar.Event) $R2 = {
  id = "e1"
  name = "Event 1"
  time = (start = 2024-01-03 17:00:00 UTC, duration = Swift.Double @ 0x0000600001754928)
  count = (_value = 0)
}
> $R2.count + 2
2

Ah, this looks like swift's description generator actually prints out (_value = 0) when it is part of a larger struct that has a description. Nothing you can do about this.

My main concern is that the SBValue::GetDescription() can create multi-line output, and as you have discovered, when displayed in the GUI, it will chop off the value at the first newline which makes this less useful. I am not sure how I feel about this as this description could be quite large and it isn't doing much better than what used to be there before with the <typename> @ address description, now we will get (PointType) pt= { which isn't much better.

@clayborg
Copy link
Collaborator

Is there a way we can tell that the request is from the console and only enable this feature if we are going to dump it to the debug console?

@walter-erquinigo
Copy link
Member

Now that I think of it, it would be really nice if GetDescription() could get a one_line flag that prints the description as a single line. Then the rendering will be much better on vscode, because we already dump the children of each object anyway.

@ashgti
Copy link
Contributor Author

ashgti commented Jan 10, 2024

Is there a way we can tell that the request is from the console and only enable this feature if we are going to dump it to the debug console?

Yea, the DAP has a context we can use to limit this https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Evaluate

  /**
   * The context in which the evaluate request is used.
   * Values:
   * 'watch': evaluate is called from a watch view context.
   * 'repl': evaluate is called from a REPL context.
   * 'hover': evaluate is called to generate the debug hover contents.
   * This value should only be used if the corresponding capability
   * `supportsEvaluateForHovers` is true.
   * 'clipboard': evaluate is called to generate clipboard contents.
   * This value should only be used if the corresponding capability
   * `supportsClipboardContext` is true.
   * 'variables': evaluate is called from a variables view context.
   * etc.
   */
  context?: 'watch' | 'repl' | 'hover' | 'clipboard' | 'variables' | string;

I'll work on an update to this logic to only use GetDescription for hover and repl cases.

@clayborg
Copy link
Collaborator

Is there a way we can tell that the request is from the console and only enable this feature if we are going to dump it to the debug console?

Yea, the DAP has a context we can use to limit this https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Evaluate

  /**
   * The context in which the evaluate request is used.
   * Values:
   * 'watch': evaluate is called from a watch view context.
   * 'repl': evaluate is called from a REPL context.
   * 'hover': evaluate is called to generate the debug hover contents.
   * This value should only be used if the corresponding capability
   * `supportsEvaluateForHovers` is true.
   * 'clipboard': evaluate is called to generate clipboard contents.
   * This value should only be used if the corresponding capability
   * `supportsClipboardContext` is true.
   * 'variables': evaluate is called from a variables view context.
   * etc.
   */
  context?: 'watch' | 'repl' | 'hover' | 'clipboard' | 'variables' | string;

I'll work on an update to this logic to only use GetDescription for hover and repl cases.

That will work nicely.

@ashgti
Copy link
Contributor Author

ashgti commented Jan 11, 2024

That will work nicely.

Done, the latest revision will only use the description for hovers and repl contexts.

@Endilll Endilll removed their request for review January 11, 2024 21:40
lldb/source/API/SBValue.cpp Outdated Show resolved Hide resolved
lldb/tools/lldb-dap/JSONUtils.h Outdated Show resolved Hide resolved
os << separator << name << ":" << value;
separator = ", ";
}
desc = "{...}"; // Fallback for nested types.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"{...}" should only be used for structs unions or classes. We are processing the children of an item. You will need to check the type class so I would recommend:

static bool IsClassStructOrUnionType(lldb::SBType t) {
  return (t.GetTypeClass() & (lldb::eTypeClassUnion | lldb::eTypeClassStruct | lldb::eTypeClassUnion)) != 0
}

Then in the above code:

  if (IsClassStructOrUnionType(child.GetType())
    desc = "{...}"

It seems we would want this description for a top level class/struct/union as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this to use the {...} for nested types.

…over and

repl DAP evaluate requests.

When generating a `display_value` for a variable the current approach calls
`SBValue::GetValue()` and `SBValue::GetSummary()` to generate a `display_value`
for the `SBValue`. However, there are cases where both of these return an empty
string and the fallback is to print a pointer and type name instead (e.g.
"FooBarType @ 0x00321").

For swift types, lldb includes a langauge runtime plugin that can generate a
user description of the object but this is only used with
`SBValue::GetDescription()`.

For example:
```
$ lldb swift-binary
... stop at breakpoint ...
lldb> script
>>> event = lldb.frame.GetValueForVariablePath("event")
>>> print("Value", event.GetValue())
Value None
>>> print("Summary", event.GetSummary())
Summary None
>>> print("Description", event)
Description (main.Event) event = (name = "Greetings", time = 2024-01-04 23:38:06 UTC)
```

With this change we now call SBValue::GetDescription() which will internally
include a summary (if configured) or call into a language runtime to return
a formatted description of the value.
@DavidGoldman DavidGoldman merged commit 40a361a into llvm:main Jan 12, 2024
4 checks passed
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
…fallback. (llvm#77026)

When generating a `display_value` for a variable the current approach
calls `SBValue::GetValue()` and `SBValue::GetSummary()` to generate a
`display_value` for the `SBValue`. However, there are cases where both
of these return an empty string and the fallback is to print a pointer
and type name instead (e.g. `FooBarType @ 0x00321`).

For swift types, lldb includes a langauge runtime plugin that can
generate a description of the object but this is only used with
`SBValue::GetDescription()`.

For example:
```
$ lldb swift-binary
... stop at breakpoint ...
lldb> script
>>> event = lldb.frame.GetValueForVariablePath("event")
>>> print("Value", event.GetValue())
Value None
>>> print("Summary", event.GetSummary())
Summary None
>>> print("Description", event) # __str__ calls SBValue::GetDescription()
Description (main.Event) event = (name = "Greetings", time = 2024-01-04 23:38:06 UTC)
```

With this change, if GetValue and GetSummary return empty then we try
`SBValue::GetDescription()` as a fallback before using the previous
logic of printing `<type> @ <addr>`.
@labath
Copy link
Collaborator

labath commented May 14, 2024

I'm currently optimizing our data formatters for some fairly complex data structures, and I've ran into something I don't understand. My DAP packet sequence for a hover request consists of two (mostly redundant) packets:

--> 
Content-Length: 114

{
  "arguments": {
    "context": "hover",
    "expression": "v",
    "frameId": 524288
  },
  "command": "evaluate",
  "seq": 21,
  "type": "request"
}
<-- 
Content-Length: 1830

{
  "body": {
    "result": "(std::vector<int>) v = size=121 {\n  [0] = 1\n  [1] = 2\n  [2] = 3\n  [3] = 4\n  [4] = 5\n  [5] = 6\n  [6] = 7\n  [7] = 77\n  [8] = 3\n  [9] = 33\n  [10] = 54\n  [11] = 5\n  [12] = 53\n  [13] = 523\n  [14] = 532\n  [15] = 53\n  [16] = 3535\n  [17] = 53\n  [18] = 2235\n  [19] = 352\n  [20] = 35\n  [21] = 352\n  [22] = 532\n  [23] = 532\n  [24] = 53\n  [25] = 35523\n  [26] = 523\n  [27] = 532\n  [28] = 523\n  [29] = 523\n  [30] = 523\n  [31] = 523\n  [32] = 235\n  [33] = 523\n  [34] = 532\n  [35] = 532\n  [36] = 523\n  [37] = 523\n  [38] = 53\n  [39] = 312\n  [40] = 5\n  [41] = 346\n  [42] = 4256\n  [43] = 346\n  [44] = 234\n  [45] = 632\n  [46] = 523\n  [47] = 6\n  [48] = 246\n  [49] = 54\n  [50] = 672\n  [51] = 67\n  [52] = 2457\n  [53] = 234\n  [54] = 7524\n  [55] = 7\n  [56] = 4376\n  [57] = 23467\n  [58] = 432\n  [59] = 6\n  [60] = 2347\n  [61] = 5435\n  [62] = 2\n  [63] = 346\n  [64] = 4536\n  [65] = 34\n  [66] = 53\n  [67] = 324\n  [68] = 645\n  [69] = 645\n  [70] = 456\n  [71] = 75\n  [72] = 343\n  [73] = 3\n  [74] = 4\n  [75] = 654\n  [76] = 3545\n  [77] = 5\n  [78] = 4\n  [79] = 435\n  [80] = 5\n  [81] = 345\n  [82] = 54\n  [83] = 435\n  [84] = 43\n  [85] = 245\n  [86] = 54\n  [87] = 32\n  [88] = 4565\n  [89] = 65\n  [90] = 54\n  [91] = 43\n  [92] = 54\n  [93] = 535\n  [94] = 3\n  [95] = 4\n  [96] = 5\n  [97] = 5\n  [98] = 65\n  [99] = 1342\n  [100] = 43657\n  [101] = 67856\n  [102] = 523\n  [103] = 4\n  [104] = 534\n  [105] = 576\n  [106] = 57\n  [107] = 46\n  [108] = 24\n  [109] = 6\n  [110] = 568\n  [111] = 5658\n  [112] = 76\n  [113] = 42\n  [114] = 634\n  [115] = 8\n  [116] = 54657\n  [117] = 6\n  [118] = 4\n  [119] = 8356\n  [120] = 75\n}",
    "type": "std::vector<int>",
    "variablesReference": 8
  },
  "command": "evaluate",
  "request_seq": 21,
  "seq": 0,
  "success": true,
  "type": "response"
}
--> 
Content-Length: 86

{
  "arguments": {
    "variablesReference": 8
  },
  "command": "variables",
  "seq": 22,
  "type": "request"
}
<-- 
Content-Length: 17549

{
  "body": {
    "variables": [
      {
        "$__lldb_extensions": {
          "value": "1"
        },
        "evaluateName": "v._M_impl._M_start->[0]",
        "name": "[0]",
        "type": "int",
        "value": "1",
        "variablesReference": 0
      },
// ...
      {
        "$__lldb_extensions": {
          "value": "75"
        },
        "evaluateName": "v._M_impl._M_start->[120]",
        "name": "[120]",
        "type": "int",
        "value": "75",
        "variablesReference": 0
      }
    ]
  },
  "command": "variables",
  "request_seq": 22,
  "seq": 0,
  "success": true,
  "type": "response"
}

The first response contains the contents of the formatted variable (a vector, in this example) as a string, while the second one contains the same data in a structured form. Looking at the history, I've found this PR, which seems to indicate that the first packet is necessary in order to provide the contents of data structure in the hover window. However, that does not appear to match what I'm seeing:

sc-0

From what I can tell by this screenshot, the only part that VSCode uses from the first response is the first line (which it uses as the "summary" of the value, while the rest of the data (the vector contents) comes from the second response.

The first response is problematic for me, because that output can contain a lot more information (if the vector contained structures rather than ints, those structures would get expanded recursively) than what's displayed on the screen, which makes the popups slower than necessary. From what I'm seeing, it would be ideal to remove the expansion in the first response completely (which would essentially amount to reverting this PR), given that the information is already contained in the second one. However, I am assuming there is a reason for why that code exists -- but then I don't understand why does my experience (and screenshots) differ so much from the screenshots on this PR. Could something have changed (over the last 4 months) in how VSCode behaves that would mean this PR is not necessary?

Can anyone shed any light on this?

@walter-erquinigo
Copy link
Member

I don't think anything has changed on VSCode proper. I've just verified I have the same experience as you. Given what you said, I'm in favor of reverting this or at least gating this feature under a json initialization option until the original author can look at this.

@labath
Copy link
Collaborator

labath commented May 15, 2024

Thanks for checking this out. I'll try to whip something up tomorrow.

@ashgti
Copy link
Contributor Author

ashgti commented May 17, 2024

I just checked and I'm not seeing the hover's in the same format as they were when I made the pull request. The expression context should still have the expanded forms though for example:

Screenshot 2024-05-16 at 5 25 14 PM

I think the VSCode team is working on making the Debug view more extensible for visualizations, see https://github.com/microsoft/vscode/blob/main/src/vscode-dts/vscode.proposed.debugVisualization.d.ts and microsoft/vscode#197287

@labath
Copy link
Collaborator

labath commented May 17, 2024

Thanks for checking this out.

I just checked and I'm not seeing the hover's in the same format as they were when I made the pull request. The expression context should still have the expanded forms though for example:
Screenshot 2024-05-16 at 5 25 14 PM

I see. I think it makes sense to show this expanded view in the debug console view. However, it does not appear to be useful for the hover view, and it's slowing things down significantly (for one particular probobuf variable, this increased the hover time from <1sec to ~6-7 seconds).

Would you be ok (both of you) with limiting the scope of this patch to the "repl" context?

@walter-erquinigo
Copy link
Member

I'm okay with anything that ensures hovering is fast.

@ashgti
Copy link
Contributor Author

ashgti commented May 17, 2024

Sounds good to me

labath added a commit to labath/llvm-project that referenced this pull request May 20, 2024
VSCode will automatically ask for the children (in structured form) so
there's no point in sending the textual representation. This can make
displaying hover popups for complex variables with complicated data
formatters much faster. See discussion on llvm#77026 for context.
labath added a commit that referenced this pull request May 21, 2024
…92726)

VSCode will automatically ask for the children (in structured form) so
there's no point in sending the textual representation. This can make
displaying hover popups for complex variables with complicated data
formatters much faster. See discussion on #77026 for context.
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.

6 participants