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

walter-erquinigo · 2023-09-15T20:43:19Z

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.

llvmbot · 2023-09-15T20:44:26Z

@llvm/pr-subscribers-lldb

Changes

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.

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

6 Files Affected:

(modified) lldb/include/lldb/Target/Target.h (+2)
(modified) lldb/source/Core/DumpDataExtractor.cpp (+11-4)
(modified) lldb/source/Target/Target.cpp (+8-2)
(modified) lldb/source/Target/TargetProperties.td (+3)
(modified) lldb/test/API/python_api/value/TestValueAPI.py (+16-1)
(modified) lldb/test/API/python_api/value/main.c (+5-2)

diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index ed0ecbbddbf8149..b2dcd3b0b3d30a0 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -167,6 +167,8 @@ class TargetProperties : public Properties {
 
   bool GetEnableSyntheticValue() const;
 
+  bool ShouldShowHexValuesWithLeadingZeroes() const;
+
   uint32_t GetMaxZeroPaddingInFloatFormat() const;
 
   uint32_t GetMaximumNumberOfChildrenToDisplay() const;
diff --git a/lldb/source/Core/DumpDataExtractor.cpp b/lldb/source/Core/DumpDataExtractor.cpp
index cb76b118325b7ef..fa9c5209ed490ff 100644
--- a/lldb/source/Core/DumpDataExtractor.cpp
+++ b/lldb/source/Core/DumpDataExtractor.cpp
@@ -620,10 +620,17 @@ lldb::offset_t lldb_private::DumpDataExtractor(
       case 2:
       case 4:
       case 8:
-        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));
+        if (exe_scope->CalculateTarget()
+                ->ShouldShowHexValuesWithLeadingZeroes()) {
+          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;
       default: {
         assert(item_bit_size == 0 && item_bit_offset == 0);
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index 35f5ef324fde667..87746d7aa3c32aa 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -4236,8 +4236,8 @@ bool TargetProperties::SetPreferDynamicValue(lldb::DynamicValueType d) {
 }
 
 bool TargetProperties::GetPreloadSymbols() const {
-  if (INTERRUPT_REQUESTED(m_target->GetDebugger(), 
-      "Interrupted checking preload symbols")) {
+  if (INTERRUPT_REQUESTED(m_target->GetDebugger(),
+                          "Interrupted checking preload symbols")) {
     return false;
   }
   const uint32_t idx = ePropertyPreloadSymbols;
@@ -4539,6 +4539,12 @@ bool TargetProperties::GetEnableSyntheticValue() const {
       idx, g_target_properties[idx].default_uint_value != 0);
 }
 
+bool TargetProperties::ShouldShowHexValuesWithLeadingZeroes() const {
+  const uint32_t idx = ePropertyShowHexValuesWithLeadingZeroes;
+  return GetPropertyAtIndexAs<bool>(
+      idx, g_target_properties[idx].default_uint_value != 0);
+}
+
 uint32_t TargetProperties::GetMaxZeroPaddingInFloatFormat() const {
   const uint32_t idx = ePropertyMaxZeroPaddingInFloatFormat;
   return GetPropertyAtIndexAs<uint64_t>(
diff --git a/lldb/source/Target/TargetProperties.td b/lldb/source/Target/TargetProperties.td
index 3bfdfa8cfaa957f..9f639842a80841a 100644
--- a/lldb/source/Target/TargetProperties.td
+++ b/lldb/source/Target/TargetProperties.td
@@ -82,6 +82,9 @@ let Definition = "target" in {
   def SaveObjectsDir: Property<"save-jit-objects-dir", "FileSpec">,
     DefaultStringValue<"">,
     Desc<"If specified, the directory to save intermediate object files generated by the LLVM JIT">;
+  def ShowHexValuesWithLeadingZeroes: Property<"show-hex-values-with-leading-zeroes", "Boolean">,
+    DefaultTrue,
+    Desc<"Whether to display leading zeroes in hex ValueObject values.">;
   def MaxZeroPaddingInFloatFormat: Property<"max-zero-padding-in-float-format", "UInt64">,
     DefaultUnsignedValue<6>,
     Desc<"The maximum number of zeroes to insert when displaying a very small float before falling back to scientific notation.">;
diff --git a/lldb/test/API/python_api/value/TestValueAPI.py b/lldb/test/API/python_api/value/TestValueAPI.py
index eb709ca4728b089..05670b8db9bb77a 100644
--- a/lldb/test/API/python_api/value/TestValueAPI.py
+++ b/lldb/test/API/python_api/value/TestValueAPI.py
@@ -3,9 +3,9 @@
 """
 
 import lldb
+from lldbsuite.test import lldbutil
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
-from lldbsuite.test import lldbutil
 
 
 class ValueAPITestCase(TestBase):
@@ -206,3 +206,18 @@ def test(self):
             -526164208,
             "signed sinthex == -526164208",
         )
+
+        # Check that hex value printing works as expected.
+        self.assertEqual(
+            frame0.FindVariable("fixed_int_ptr").GetValue(),
+            "0x00000000000000aa",
+        )
+        self.runCmd("settings set target.show-hex-values-with-leading-zeroes false")
+        self.assertEqual(
+            frame0.FindVariable("another_fixed_int_ptr").GetValue(),
+            "0xaa",
+        )
+        self.assertEqual(
+            frame0.FindVariable("a_null_int_ptr").GetValue(),
+            "0x0",
+        )
diff --git a/lldb/test/API/python_api/value/main.c b/lldb/test/API/python_api/value/main.c
index bf00aba07661863..672b0df376dc5a2 100644
--- a/lldb/test/API/python_api/value/main.c
+++ b/lldb/test/API/python_api/value/main.c
@@ -44,13 +44,16 @@ int main (int argc, char const *argv[])
     int i;
     MyInt a = 12345;
     struct MyStruct s = { 11, 22 };
-    struct MyBiggerStruct f = { 33, 44, 55 }; 
+    struct MyBiggerStruct f = { 33, 44, 55 };
     int *my_int_ptr = &g_my_int;
     printf("my_int_ptr points to location %p\n", my_int_ptr);
+    int *fixed_int_ptr = (int*)(void*)0xAA;
+    int *another_fixed_int_ptr = (int*)(void*)0xAA;
+    int *a_null_int_ptr = NULL;
     const char **str_ptr = days_of_week;
     for (i = 0; i < 7; ++i)
         printf("%s\n", str_ptr[i]); // Break at this line
                                     // and do str_ptr_val.GetChildAtIndex(5, lldb.eNoDynamicValues, True).
-    
+
     return 0;
 }

clayborg

Nice! LGTM

clayborg · 2023-09-15T20:47:31Z

lldb/source/Core/DumpDataExtractor.cpp

-                  (int)(2 * item_byte_size), (int)(2 * item_byte_size),
-                  DE.GetMaxU64Bitfield(&offset, item_byte_size, item_bit_size,
-                                       item_bit_offset));
+        if (exe_scope->CalculateTarget()


Actually do you need to test if "exe_scope" is not NULL here?

jimingham · 2023-09-15T20:55:38Z

There are other places we print hex numbers that don't go through the DataExtractor. For instance, the "location" for a ValueObject is printed directly, so this change wouldn't catch it. It's a little off-putting if you set a global setting to "not print leading zeros" and then sometimes we still print leading zeros.

This is not an objection to the contents of this patch, but I think to be complete we're going to end up finding more places where we should be checking this policy setting.

walter-erquinigo · 2023-09-15T21:09:31Z

@jimingham , that's a good catch. Probably I should rename this because I want this setting to affect only SBValueObject::GetValue(), which is want ends up being sent to IDEs as a common interface. Trying to control all places that print as hex value is a bit too much and it'll be hard to enforce in the long term. Any particular suggestion? Because I can't really come up with a good name better than show-leading-zeroes-in-valueobject-values.

jimingham · 2023-09-15T21:18:32Z

That really should be show-leading-zeros-in-valueobject-variables-using-hex-format, shouldn't it? I think you can actually just document what it actually does in the help string and then you shouldn't have to make the name overly pedantic. The one you have already is probably enough.

clayborg · 2023-09-15T21:20:22Z

@jimingham , that's a good catch. Probably I should rename this because I want this setting to affect only SBValueObject::GetValue(), which is want ends up being sent to IDEs as a common interface. Trying to control all places that print as hex value is a bit too much and it'll be hard to enforce in the long term. Any particular suggestion? Because I can't really come up with a good name better than show-leading-zeroes-in-valueobject-values.

I think the setting name is ok as is. No one outside of LLDB developers have any idea what "valueobject" means. Also this setting is "show-hex-values-with-leading-zeroes" which does mention "values" so it can be ok for not all hex values emitted that are not values to have leading zeroes. The location is not a "value" technically and we might want some values coming out with leading zeroes like any command that dumps out table style output like "image dymp symtab" and other areas. Maybe we can name this setting:

show-hex-variable-values-with-leading-zeroes

The confirm this is for variable values only and not a system wide thing?

If we switch the setting to be global in the target settings we can get the global target settings without needing to check the exe_scope. Thoughts?

walter-erquinigo · 2023-09-15T21:27:21Z

I like Greg's proposal of naming it show-hex-variable-values-with-leading-zeroes. Seems very clear. I also like the idea of making it a global target config, which would simplify the code.

…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.

walter-erquinigo · 2023-09-15T21:46:39Z

I've updated the PR following Greg's suggestions.

clayborg

LGTM. Jim? You ok with this?

jimingham · 2023-09-18T16:47:27Z

Sure.

walter-erquinigo · 2023-09-18T16:48:11Z

Thanks Jim and Greg!

…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.

DavidSpickett · 2023-09-19T07:50:31Z

This is failing on Arm 32 bit (https://lab.llvm.org/buildbot/#/builders/17/builds/43300), I'll reproduce and fix it. I expect it just needs to expect 8 hex characters instead of 16.

DavidSpickett · 2023-09-19T08:53:33Z

9568601

walter-erquinigo · 2023-09-19T12:23:21Z

Thanks, man!

…

On Tue, Sep 19, 2023, 4:53 AM David Spickett ***@***.***> wrote: 9568601 <9568601> — Reply to this email directly, view it on GitHub <#66548 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMKAMXBPHZKLWFBKO3EMPDX3FMRTANCNFSM6AAAAAA42J72F4> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

…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 requested a review from clayborg September 15, 2023 20:43

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

clayborg approved these changes Sep 15, 2023

View reviewed changes

clayborg requested changes Sep 15, 2023

View reviewed changes

walter-erquinigo force-pushed the walter/hex branch from 750c856 to 99b2a5e Compare September 15, 2023 20:53

walter-erquinigo changed the title ~~[LLDB] Add a setting for print hex values without leading zeroes~~ [LLDB] Add a setting for printing ValueObject hex values without leading zeroes Sep 15, 2023

walter-erquinigo force-pushed the walter/hex branch from 99b2a5e to 3bddb5d Compare September 15, 2023 21:45

clayborg reviewed Sep 16, 2023

View reviewed changes

walter-erquinigo merged commit 710276a into llvm:main Sep 18, 2023

walter-erquinigo deleted the walter/hex branch September 18, 2023 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

walter-erquinigo commented Sep 15, 2023

llvmbot commented Sep 15, 2023

clayborg left a comment

clayborg Sep 15, 2023

jimingham commented Sep 15, 2023

walter-erquinigo commented Sep 15, 2023

jimingham commented Sep 15, 2023

clayborg commented Sep 15, 2023

walter-erquinigo commented Sep 15, 2023

walter-erquinigo commented Sep 15, 2023

clayborg left a comment

jimingham commented Sep 18, 2023

walter-erquinigo commented Sep 18, 2023

DavidSpickett commented Sep 19, 2023

DavidSpickett commented Sep 19, 2023

walter-erquinigo commented Sep 19, 2023 via email

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

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

Conversation

walter-erquinigo commented Sep 15, 2023

llvmbot commented Sep 15, 2023

clayborg left a comment

Choose a reason for hiding this comment

clayborg Sep 15, 2023

Choose a reason for hiding this comment

jimingham commented Sep 15, 2023

walter-erquinigo commented Sep 15, 2023

jimingham commented Sep 15, 2023

clayborg commented Sep 15, 2023

walter-erquinigo commented Sep 15, 2023

walter-erquinigo commented Sep 15, 2023

clayborg left a comment

Choose a reason for hiding this comment

jimingham commented Sep 18, 2023

walter-erquinigo commented Sep 18, 2023

DavidSpickett commented Sep 19, 2023

DavidSpickett commented Sep 19, 2023

walter-erquinigo commented Sep 19, 2023 via email