[lldb-vscode] Show value addresses in a short format #66534

walter-erquinigo · 2023-09-15T18:07:52Z

The variables pane on VSCode is very narrow by default, and lldb-vscode has been using the default formatter for addresses, which uses 18 characters for each address. That's a bit too much because it prints too many leading zeroes.
As a way to improve readability of variables, I'm adding some logic to format addresses manually using as few chars as possible. I don't want to mess with the default LLDB formatter because, if the user uses the debug console, they should see addresses formatted in the regular way.

I also added some logic to print when a null pointer appears, and when applicable, which is a bit more readable.

For example, this is how a default variables pane looks like:

And this is how it looks with the improvement:

llvmbot · 2023-09-15T18:09:02Z

@llvm/pr-subscribers-lldb

Changes

The variables pane on VSCode is very narrow by default, and lldb-vscode has been using the default formatter for addresses, which uses 18 characters for each address. That's a bit too much because it prints too many leading zeroes.
As a way to improve readability of variables, I'm adding some logic to format addresses manually using as few chars as possible. I don't want to mess with the default LLDB formatter because, if the user uses the debug console, they should see addresses formatted in the regular way.

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

6 Files Affected:

(modified) lldb/include/lldb/API/SBType.h (+4)
(modified) lldb/source/API/SBType.cpp (+4)
(modified) lldb/test/API/tools/lldb-vscode/evaluate/main.cpp (+1-1)
(modified) lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py (+93-14)
(modified) lldb/test/API/tools/lldb-vscode/variables/main.cpp (+6)
(modified) lldb/tools/lldb-vscode/JSONUtils.cpp (+19-2)

diff --git a/lldb/include/lldb/API/SBType.h b/lldb/include/lldb/API/SBType.h
index 5962f0c50dee14f..c8fcb759dede6b2 100644
--- a/lldb/include/lldb/API/SBType.h
+++ b/lldb/include/lldb/API/SBType.h
@@ -121,6 +121,10 @@ class SBType {
 
   uint64_t GetByteSize();
 
+  /// \return
+  ///    Whether the type is a pointer or a reference.
+  bool IsPointerOrReferenceType();
+
   bool IsPointerType();
 
   bool IsReferenceType();
diff --git a/lldb/source/API/SBType.cpp b/lldb/source/API/SBType.cpp
index ee5b6447428098e..38fcb54f5ea98dc 100644
--- a/lldb/source/API/SBType.cpp
+++ b/lldb/source/API/SBType.cpp
@@ -127,6 +127,10 @@ uint64_t SBType::GetByteSize() {
   return 0;
 }
 
+bool SBType::IsPointerOrReferenceType() {
+  return IsPointerType() || IsReferenceType();
+}
+
 bool SBType::IsPointerType() {
   LLDB_INSTRUMENT_VA(this);
 
diff --git a/lldb/test/API/tools/lldb-vscode/evaluate/main.cpp b/lldb/test/API/tools/lldb-vscode/evaluate/main.cpp
index 3a541b21b220828..bed853ba6e1433e 100644
--- a/lldb/test/API/tools/lldb-vscode/evaluate/main.cpp
+++ b/lldb/test/API/tools/lldb-vscode/evaluate/main.cpp
@@ -43,6 +43,6 @@ int main(int argc, char const *argv[]) {
   my_bool_vec.push_back(true);
   my_bool_vec.push_back(false); // breakpoint 6
   my_bool_vec.push_back(true); // breakpoint 7
-  
+
   return 0;
 }
diff --git a/lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py b/lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
index fc24b3b34e70283..efb5f3ec284589f 100644
--- a/lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
+++ b/lldb/test/API/tools/lldb-vscode/variables/TestVSCode_variables.py
@@ -42,6 +42,17 @@ def verify_values(self, verify_dict, actual, varref_dict=None, expression=None):
                     ('"%s" value "%s" doesn\'t start with' ' "%s")')
                     % (key, actual_value, verify_value),
                 )
+        if "notstartswith" in verify_dict:
+            verify = verify_dict["notstartswith"]
+            for key in verify:
+                verify_value = verify[key]
+                actual_value = actual[key]
+                startswith = actual_value.startswith(verify_value)
+                self.assertFalse(
+                    startswith,
+                    ('"%s" value "%s" starts with' ' "%s")')
+                    % (key, actual_value, verify_value),
+                )
         if "contains" in verify_dict:
             verify = verify_dict["contains"]
             for key in verify:
@@ -155,6 +166,7 @@ def do_test_scopes_variables_setVariable_evaluate(
             "argv": {
                 "equals": {"type": "const char **"},
                 "startswith": {"value": "0x"},
+                "notstartswith": {"value": "0x0"},
                 "hasVariablesReference": True,
             },
             "pt": {
@@ -166,8 +178,53 @@ def do_test_scopes_variables_setVariable_evaluate(
                     "buffer": {"children": buffer_children},
                 },
             },
+            "pt_ptr": {
+                "equals": {"type": "PointType *"},
+                "startswith": {"value": "0x"},
+                "notstartswith": {"value": "0x0"},
+                "hasVariablesReference": True,
+            },
+            "another_pt_ptr": {
+                "equals": {"type": "PointType *"},
+                "startswith": {"value": "<null>"},
+                "hasVariablesReference": True,
+            },
             "x": {"equals": {"type": "int"}},
+            "some_int": {
+                "equals": {
+                    "type": "int",
+                    "value": "10",
+                },
+            },
+            "some_int_ptr": {
+                "equals": {"type": "int *"},
+                "startswith": {"value": "0x"},
+                "notstartswith": {"value": "0x0"},
+            },
+            "another_int_ptr": {
+                "equals": {
+                    "type": "int *",
+                    "value": "<null>",
+                },
+            },
         }
+        if enableAutoVariableSummaries:
+            verify_locals["pt_ptr"] = {
+                "equals": {"type": "PointType *"},
+                "hasVariablesReference": True,
+                "children": {
+                    "x": {"equals": {"type": "int", "value": "11"}},
+                    "y": {"equals": {"type": "int", "value": "22"}},
+                    "buffer": {"children": buffer_children},
+                },
+            }
+            verify_locals["some_int_ptr"] ={
+                "equals": {
+                    "type": "int *",
+                    "value": "20",
+                },
+            }
+
         verify_globals = {
             "s_local": {"equals": {"type": "float", "value": "2.25"}},
             "::g_global": {"equals": {"type": "int", "value": "123"}},
@@ -297,9 +354,9 @@ def do_test_scopes_variables_setVariable_evaluate(
 
         verify_locals["argc"]["equals"]["value"] = "123"
         verify_locals["pt"]["children"]["x"]["equals"]["value"] = "111"
-        verify_locals["x @ main.cpp:17"] = {"equals": {"type": "int", "value": "89"}}
-        verify_locals["x @ main.cpp:19"] = {"equals": {"type": "int", "value": "42"}}
-        verify_locals["x @ main.cpp:21"] = {"equals": {"type": "int", "value": "72"}}
+        verify_locals["x @ main.cpp:23"] = {"equals": {"type": "int", "value": "89"}}
+        verify_locals["x @ main.cpp:25"] = {"equals": {"type": "int", "value": "42"}}
+        verify_locals["x @ main.cpp:27"] = {"equals": {"type": "int", "value": "72"}}
 
         self.verify_variables(verify_locals, self.vscode.get_local_variables())
 
@@ -310,29 +367,29 @@ def do_test_scopes_variables_setVariable_evaluate(
         )
 
         self.assertTrue(
-            self.vscode.request_setVariable(1, "x @ main.cpp:17", 17)["success"]
+            self.vscode.request_setVariable(1, "x @ main.cpp:23", 17)["success"]
         )
         self.assertTrue(
-            self.vscode.request_setVariable(1, "x @ main.cpp:19", 19)["success"]
+            self.vscode.request_setVariable(1, "x @ main.cpp:25", 19)["success"]
         )
         self.assertTrue(
-            self.vscode.request_setVariable(1, "x @ main.cpp:21", 21)["success"]
+            self.vscode.request_setVariable(1, "x @ main.cpp:27", 21)["success"]
         )
 
         # The following should have no effect
         self.assertFalse(
-            self.vscode.request_setVariable(1, "x @ main.cpp:21", "invalid")["success"]
+            self.vscode.request_setVariable(1, "x @ main.cpp:27", "invalid")["success"]
         )
 
-        verify_locals["x @ main.cpp:17"]["equals"]["value"] = "17"
-        verify_locals["x @ main.cpp:19"]["equals"]["value"] = "19"
-        verify_locals["x @ main.cpp:21"]["equals"]["value"] = "21"
+        verify_locals["x @ main.cpp:23"]["equals"]["value"] = "17"
+        verify_locals["x @ main.cpp:25"]["equals"]["value"] = "19"
+        verify_locals["x @ main.cpp:27"]["equals"]["value"] = "21"
 
         self.verify_variables(verify_locals, self.vscode.get_local_variables())
 
         # The plain x variable shold refer to the innermost x
         self.assertTrue(self.vscode.request_setVariable(1, "x", 22)["success"])
-        verify_locals["x @ main.cpp:21"]["equals"]["value"] = "22"
+        verify_locals["x @ main.cpp:27"]["equals"]["value"] = "22"
 
         self.verify_variables(verify_locals, self.vscode.get_local_variables())
 
@@ -349,9 +406,9 @@ def do_test_scopes_variables_setVariable_evaluate(
         names = [var["name"] for var in locals]
         # The first shadowed x shouldn't have a suffix anymore
         verify_locals["x"] = {"equals": {"type": "int", "value": "17"}}
-        self.assertNotIn("x @ main.cpp:17", names)
-        self.assertNotIn("x @ main.cpp:19", names)
-        self.assertNotIn("x @ main.cpp:21", names)
+        self.assertNotIn("x @ main.cpp:23", names)
+        self.assertNotIn("x @ main.cpp:25", names)
+        self.assertNotIn("x @ main.cpp:27", names)
 
         self.verify_variables(verify_locals, locals)
 
@@ -421,10 +478,32 @@ def do_test_scopes_and_evaluate_expansion(self, enableAutoVariableSummaries: boo
                     },
                 },
             },
+            "pt_ptr": {
+                "equals": {"type": "PointType *"},
+                "hasVariablesReference": True,
+                "missing": ["indexedVariables"],
+            },
+            "another_pt_ptr": {
+                "equals": {"type": "PointType *"},
+                "hasVariablesReference": True,
+                "missing": ["indexedVariables"],
+            },
             "x": {
                 "equals": {"type": "int"},
                 "missing": ["indexedVariables"],
             },
+            "some_int": {
+                "equals": {"type": "int"},
+                "missing": ["indexedVariables"],
+            },
+            "some_int_ptr": {
+                "equals": {"type": "int *"},
+                "missing": ["indexedVariables"],
+            },
+            "another_int_ptr": {
+                "equals": {"type": "int *"},
+                "missing": ["indexedVariables"],
+            },
         }
         self.verify_variables(verify_locals, locals)
 
diff --git a/lldb/test/API/tools/lldb-vscode/variables/main.cpp b/lldb/test/API/tools/lldb-vscode/variables/main.cpp
index d81a9a20544a856..10e3459e1c1fb25 100644
--- a/lldb/test/API/tools/lldb-vscode/variables/main.cpp
+++ b/lldb/test/API/tools/lldb-vscode/variables/main.cpp
@@ -12,8 +12,14 @@ int test_indexedVariables();
 int main(int argc, char const *argv[]) {
   static float s_local = 2.25;
   PointType pt = { 11,22, {0}};
+  PointType *pt_ptr = new PointType{11, 22, {0}};
+  PointType *another_pt_ptr = nullptr;
   for (int i=0; i<BUFFER_SIZE; ++i)
     pt.buffer[i] = i;
+
+  int some_int = 10;
+  int *some_int_ptr = new int{20};
+  int *another_int_ptr = nullptr;
   int x = s_global - g_global - pt.y; // breakpoint 1
   {
     int x = 42;
diff --git a/lldb/tools/lldb-vscode/JSONUtils.cpp b/lldb/tools/lldb-vscode/JSONUtils.cpp
index c6b422e4d7a02e6..30c35b9e52c8404 100644
--- a/lldb/tools/lldb-vscode/JSONUtils.cpp
+++ b/lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -200,7 +200,7 @@ static bool ShouldBeDereferencedForSummary(lldb::SBValue &v) {
   if (!g_vsc.enable_auto_variable_summaries)
     return false;
 
-  if (!v.GetType().IsPointerType() && !v.GetType().IsReferenceType())
+  if (!v.GetType().IsPointerOrReferenceType())
     return false;
 
   // If we are referencing a pointer, we don't dereference to avoid confusing
@@ -228,7 +228,24 @@ void SetValueForKey(lldb::SBValue &v, llvm::json::Object &object,
     strm << "<error: " << error.GetCString() << ">";
   } else {
     auto tryDumpSummaryAndValue = [&strm](lldb::SBValue value) {
-      llvm::StringRef val = value.GetValue();
+      std::string val;
+      // Whenever the value is a non-synthetic address, we format it ourselves
+      // to use as few chars as possible because the variables pane on VS Code
+      // is by default narrow.
+      if (!value.IsSynthetic() && value.GetType().IsPointerOrReferenceType()) {
+        lldb::addr_t address = value.GetValueAsUnsigned(LLDB_INVALID_ADDRESS);
+        if (address == LLDB_INVALID_ADDRESS) {
+          val = "<invalid address>";
+        } else if (address == 0) {
+          val = "<null>";
+        } else {
+          llvm::raw_string_ostream os(val);
+          os << llvm::format_hex(address, 0);
+        }
+      } else {
+        val = llvm::StringRef(value.GetValue()).str();
+      }
+
       llvm::StringRef summary = value.GetSummary();
       if (!val.empty()) {
         strm << val;

The variables pane on VSCode is very narrow by default, and lldb-vscode has been using the default formatter for addresses, which uses 18 characters for each address. That's a bit too much because it prints too many leading zeroes. As a way to improve readability of variables, I'm adding some logic to format addresses manually using as few chars as possible. I don't want to mess with the default LLDB formatter because, if the user uses the debug console, they should see addresses formatted in the regular way.

clayborg

I would rather we add a new setting in lldb that controls if we see hex values with leading zeroes and allow users to set this. It will be very easy to fix this as all you need to do is fix lldb_private::DumpDataExtractor() and change the "case eFormatHex:" case.

Setting could be:

(lldb) settings set target.show-hex-values-with-leading-zeroes true
(lldb) settings set target.show-hex-values-with-leading-zeroes false

Then users can choose how they want to see their values and it will be consistent across the command line and IDEs

clayborg · 2023-09-15T19:28:10Z

lldb/tools/lldb-vscode/JSONUtils.cpp

+        if (address == LLDB_INVALID_ADDRESS) {
+          val = "<invalid address>";
+        } else if (address == 0) {
+          val = "<null>";
+        } else {
+          llvm::raw_string_ostream os(val);
+          os << llvm::format_hex(address, 0);
+        }


I wouldn't mess with the values or try to display them in any fancy way, I would always use:

llvm::raw_string_ostream os(val); os << llvm::format_hex(address, 0);

Users might encode -1 into their pointers as a special value and you wouldn't want to see "" as the value. Also I would rather see "0x0" instead of "".

clayborg · 2023-09-15T19:50:43Z

I have verified that if I change DumpDataExtractor() you can easily do this:

    case eFormatHex:
    case eFormatHexUppercase: {
      bool wantsuppercase = (item_format == eFormatHexUppercase);
      switch (item_byte_size) {
      case 1:
      case 2:
      case 4:
      case 8:
        if (/*check if leading zeroes is enabled*/) {
          s->Printf(wantsuppercase ? "0x%*.*" PRIX64 : "0x%*.*" PRIx64,
                    (int)(2 * item_byte_size), (int)(2 * item_byte_size),
                    DE.GetMaxU64Bitfield(&offset, item_byte_size, item_bit_size,
                                         item_bit_offset));
        } else {
          s->Printf(wantsuppercase ? "0x%" PRIX64 : "0x%" PRIx64,
                    DE.GetMaxU64Bitfield(&offset, item_byte_size, item_bit_size,
                                         item_bit_offset));
        }
        break;

clayborg · 2023-09-15T19:53:30Z

If you create a setting, be sure to make it a global setting (a bool in the setting definition) so it can always be accessed via:

  static TargetProperties &Target::GetGlobalProperties();

clayborg · 2023-09-15T19:54:25Z

Then no changes are needed for vs code files at all and we can test this with a normal LLDB variable tests.

walter-erquinigo · 2023-09-15T19:59:58Z

I initially didn't want to mess with global configurations, but after a second thought, I think your solution is better. I'll try that out

…roes As suggested by Greg in llvm#66534, I'm adding a setting at the Target level that controls whether to show leading zeroes in hex ValueObject values. This has the benefit of reducing the amount of characters displayed in certain interfaces, like VSCode.

…ing zeroes (#66548) As suggested by Greg in #66534, I'm adding a setting at the Target level that controls whether to show leading zeroes in hex ValueObject values. This has the benefit of reducing the amount of characters displayed in certain interfaces, like VSCode.

…ing zeroes (llvm#66548) As suggested by Greg in llvm#66534, I'm adding a setting at the Target level that controls whether to show leading zeroes in hex ValueObject values. This has the benefit of reducing the amount of characters displayed in certain interfaces, like VSCode.

llvmbot added the lldb label Sep 15, 2023

walter-erquinigo force-pushed the walter/short-addresses branch from 82a50d6 to d8060f8 Compare September 15, 2023 18:10

walter-erquinigo requested review from River707 and clayborg September 15, 2023 18:14

walter-erquinigo marked this pull request as ready for review September 15, 2023 18:14

clayborg requested changes Sep 15, 2023

View reviewed changes

walter-erquinigo closed this Sep 15, 2023

walter-erquinigo deleted the walter/short-addresses branch September 15, 2023 20:19

walter-erquinigo mentioned this pull request Sep 15, 2023

[LLDB] Add a setting for printing ValueObject hex values without leading zeroes #66548

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb-vscode] Show value addresses in a short format #66534

[lldb-vscode] Show value addresses in a short format #66534

Uh oh!

walter-erquinigo commented Sep 15, 2023 •

edited

Loading

Uh oh!

llvmbot commented Sep 15, 2023

Uh oh!

clayborg left a comment

Uh oh!

clayborg Sep 15, 2023

Uh oh!

clayborg commented Sep 15, 2023

Uh oh!

clayborg commented Sep 15, 2023

Uh oh!

clayborg commented Sep 15, 2023

Uh oh!

walter-erquinigo commented Sep 15, 2023

Uh oh!

Uh oh!

[lldb-vscode] Show value addresses in a short format #66534

[lldb-vscode] Show value addresses in a short format #66534

Uh oh!

Conversation

walter-erquinigo commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 15, 2023

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

clayborg Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

clayborg commented Sep 15, 2023

Uh oh!

clayborg commented Sep 15, 2023

Uh oh!

clayborg commented Sep 15, 2023

Uh oh!

walter-erquinigo commented Sep 15, 2023

Uh oh!

Uh oh!

walter-erquinigo commented Sep 15, 2023 •

edited

Loading