Bugfix: Not showing the synthetic children of values behind pointers #117755

skuznetsov · 2024-11-26T18:04:46Z

This bug fix for the situation that was extensively discussed at LLVM Discourse thread: https://discourse.llvm.org/t/synthetic-data-providers-and-lldb-dap/

github-actions · 2024-11-26T18:05:05Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-11-26T18:05:40Z

@llvm/pr-subscribers-lldb

Author: Sergey Kuznetsov (skuznetsov)

Changes

This bug fix for the situation that was extensively discussed at LLVM Discourse thread: https://discourse.llvm.org/t/synthetic-data-providers-and-lldb-dap/

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

1 Files Affected:

(modified) lldb/tools/lldb-dap/lldb-dap.cpp (+7)

diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index 3bfc578806021e..b86994f60f04e5 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -4020,6 +4020,10 @@ void request_variables(DAP &dap, const llvm::json::Object &request) {
                           std::optional<std::string> custom_name = {}) {
         if (!child.IsValid())
           return;
+        if (child.IsSynthetic() && (child.GetType().IsPointerType() || child.GetType().IsReferenceType())) {
+          // Dereference to access synthetic children behind pointers/references
+          child = child.Dereference();
+        }
         bool is_permanent =
             dap.variables.IsPermanentVariableReference(variablesReference);
         int64_t var_ref = dap.variables.InsertVariable(child, is_permanent);
@@ -4028,6 +4032,9 @@ void request_variables(DAP &dap, const llvm::json::Object &request) {
             dap.enable_synthetic_child_debugging,
             /*is_name_duplicated=*/false, custom_name));
       };
+      if (variable.GetType().IsPointerType() || variable.GetType().IsReferenceType()) {
+        variable = variable.Dereference();
+      }
       const int64_t num_children = variable.GetNumChildren();
       int64_t end_idx = start + ((count == 0) ? num_children : count);
       int64_t i = start;

walter-erquinigo · 2024-11-26T18:41:06Z

lldb/tools/lldb-dap/lldb-dap.cpp

@@ -4020,6 +4020,10 @@ void request_variables(DAP &dap, const llvm::json::Object &request) {
                          std::optional<std::string> custom_name = {}) {
        if (!child.IsValid())
          return;
+        if (child.IsSynthetic() && (child.GetType().IsPointerType() || child.GetType().IsReferenceType())) {


@clayborg , what do you think about this? This pretty much matches the experience from CLI, i.e. the variable name is prepended with *, but the synthetic value is displayed right away.

walter-erquinigo · 2024-11-26T18:41:20Z

lldb/tools/lldb-dap/lldb-dap.cpp

+      if (variable.GetType().IsPointerType() || variable.GetType().IsReferenceType()) {
+        variable = variable.Dereference();
+      }


Do you know why you need this change and the one above is not enough?

The first change is more than enough. To be on the safe side, I overcomplicated it a little. I will remove the second change.

clayborg

None of this should be needed. If we have a child value that is a pointer to a synthetic value, and we expand that value, we should be showing the synthetic children via the standard LLDB APIs.

In LLDB way back in the beginning, when we would expand a pointer, we would show the first child as the dereferenced value and its name would be "*". And then if you expanded that, then you would see the contents. So if we had this code:

struct Point { int x,y; };
int my_int = 12
Point pt = {1,2};
Point *pt_ptr = &pt;
int *my_int_ptr = &i;

Old LLDB would show this as:

pt_ptr
  \_ *pt_ptr
      |- x = 1
      \- y = 2
my_int_ptr
  \_ 12

Current LLDB, when you expand a pointer, will look at the type and see if the type has children, and if it does, it will auto dereference the type:

pt_ptr
   |- x = 1
   \- y = 2
my_int_ptr
  \_ 12

And if the dereferenced value has a synthetic child provider, it should be grabbing that synthetic value and asking it for its children.

So this isn't the right fix for this. It works for C++. We need to figure out why it isn't working for your language

skuznetsov · 2024-11-26T22:31:12Z

@clayborg This is what puzzles me, too.
As I provided the examples in the Discourse thread, it works in the CLI lldb but not when used in lldb-dap.
It works only if it is dereferenced.

Also, in your example, vectors are on the top level (in the function scope).
Could it be the case that if they are a part of the structure or class, they will behave the way they do to me?

clayborg · 2024-12-02T18:14:17Z

Still works inside of a structure. Anything else I can try?

clayborg · 2024-12-02T18:15:43Z

Can you post a copy of a the python code for the synthetic child provider you are creating and also how you are registering it with LLDB?

clayborg · 2024-12-02T18:21:43Z

You should be able to debug lldb and step through the code that tries to get the synthetic child provider for the type and watch if fail. You can also step through the C++ example code from my screen shot and see how it is working to see what is going wrong and how the two things differ.

skuznetsov · 2024-12-02T19:32:45Z

Still works inside of a structure. Anything else I can try?

I tried your example and modified it in a similar way: embedded pointer to another vector, and it was working as expected.

So, I am still investigating what can be wrong here.

I am digging through the ll file for the code to find the debug declarations to recover the type.
I tried to do it via lldb, but when I type 'type lookup LavinMQ::VHostStore', it does not find it. I suspect due to the special characters "::" that are used for namespace separation.

skuznetsov · 2024-12-02T19:34:38Z

Can you post a copy of a the python code for the synthetic child provider you are creating and also how you are registering it with LLDB?

Here it is:

# Hash(K,V) class support providers
class CrystalHashSyntheticProvider:
    def __init__(self, valobj, internal_dict):
        self.valobj = valobj
        self.extract_entries()

    def has_children(self):
        return True

    def num_children(self):
        size = self.valobj.GetChildMemberWithName("size").unsigned
        return size

    def get_child_index(self, name):
        name = name.lstrip('[').rstrip(']')
        if name == "size":
            return len(self._entries) + 1 if self._entries else 1
        try:
            idx = next((index for index, obj in enumerate(self._entries) if getattr(self._entries, "key") == name), -1)
            return idx
        except:
            return -1

    def get_child_at_index(self, index):
        if index > len(self.entries):
            return None
        if index == len(self.entries):
            return self.valobj.GetChildMemberWithName("size")
        entry = self.entries[index]
        key = entry.GetChildMemberWithName("key").GetSummary()
        value = entry.GetChildMemberWithName("value").deref
        return value.CreateChildAtOffset("[%s]" % key, 0, value.type)
        # return entry.CreateChildAtOffset("[%d]" % index, 0, entry.type)

    def extract_entries(self):
        if self.valobj.type.is_pointer:
            self.valobj = self.valobj.Dereference()
        self.size = 0 if self.valobj.GetChildMemberWithName("size").value is None else self.valobj.GetChildMemberWithName("size").unsigned
        self.entries = []

        for index in range(self.size):
            try:
                valuePath = "entries[%d]" % index
                value = self.valobj.EvaluateExpression(valuePath)
                if value.type.is_pointer:
                    value = value.deref
                self.entries.append(value)

            except Exception as e:
                print('Got exception %s' % (str(e)))
                return None
        # print ("Created %d entries." % len(self.entries))

def __lldb_init_module(debugger, dict):
    # debugger.HandleCommand(r'log enable -F -s -v -f /tmp/lldb_debug.log lldb types commands')
    debugger.HandleCommand(r'type synthetic add --skip-pointers and --skip-references -l crystal_formatters.CrystalHashSyntheticProvider -x "^\(?Hash\(.+\)\)?" -w Crystal')
    debugger.HandleCommand(r'type category enable Crystal')

clayborg · 2024-12-07T00:14:05Z

You don't have a "def update(self):" method in your data formatter. This is what causes the values to be refetched. I think if you rename "extract_entries" to be named "update" it will fix your synthetic child provider.

clayborg · 2024-12-07T00:23:00Z

You also are overwriting the original lldb.SBValue with the statement self.valobj = self.valobj.Dereference() here:

    def extract_entries(self):
        if self.valobj.type.is_pointer:
            self.valobj = self.valobj.Dereference()

You never want to do this because if your value is a pointer, that pointer can change, and now you have replaced the value that is the pointer value with the first dereference value and that value will never change. Now when you run this function again, you won't get the right value because you will have locked onto the first dereferenced pointer. So if you have code like:

auto  *int_ptr = &myHash1;
int_ptr = &myHash2;

You will always be showing myHash1 as the value and it will never update. So you never touch the original self. valobj value as that is always your starting point. So lets say &myHash1 is 0x1000 and &myHash2 is 0x2000, with your code you will always dereference the pointer from 0x1000 and then you re-write your self.valobj to now always be that reference.

You also don't need to dereference the type. So your extract_entries function that currently looks like:

    def extract_entries(self):
        if self.valobj.type.is_pointer:
            self.valobj = self.valobj.Dereference()
        self.size = 0 if self.valobj.GetChildMemberWithName("size").value is None else self.valobj.GetChildMemberWithName("size").unsigned
        self.entries = []

Should be:

    def update(self):
        self.size = self.valobj.GetChildMemberWithName("size").GetValueAsUnsigned(0)
        self.entries = []

if self.valobj.GetChildMemberWithName("size") fails, it will return an invalid lldb.SBValue and calling GetValueAsUnsigned(0) on it will return the argument (0) as the fail value.

clayborg · 2024-12-07T00:27:27Z

the def update(self): is a mandatory function that is automatically called for you by the LLDB python synthetic child provider when a variable needs to be updated, so you can't rename it. Since you renamed that to be extract_entries, your synthetic child provider will never update itself. So rename this to update and call self.update() in the __init__ function, and change your function as outlined above and everything should work.

clayborg · 2024-12-07T00:29:19Z

Another thing that might not be abvious is that an instance of this class lives as long as the variable lives so as you are stepping in the same function, we will create one synthetic python instance per raw lldb.SBValue. This is why the def update(self): function is so important.

skuznetsov · 2024-12-08T14:04:07Z

@clayborg Thank you, Greg!
Lots of that info is not described anywhere, no wonder I missed it.
Let me try your fixes in my provider, and I will get back to you.

By some reason my provider works in CLI lldb, but not in lldb-dap, but I will check if your suggestions will help to fix that.

clayborg · 2024-12-10T01:50:09Z

@clayborg Thank you, Greg! Lots of that info is not described anywhere, no wonder I missed it. Let me try your fixes in my provider, and I will get back to you.

By some reason my provider works in CLI lldb, but not in lldb-dap, but I will check if your suggestions will help to fix that.

Let me know if this fixes things. I believe it will.

skuznetsov · 2025-02-01T19:05:57Z

@clayborg Thank you, Greg! Lots of that info is not described anywhere, no wonder I missed it. Let me try your fixes in my provider, and I will get back to you.
By some reason my provider works in CLI lldb, but not in lldb-dap, but I will check if your suggestions will help to fix that.

Let me know if this fixes things. I believe it will.

I tried your approach and your recommended code changes, but it does not work without my patch for some strange reason. :(

Bugfix: Not showing the synthetic children of values behind pointers

82a3bc3

skuznetsov requested a review from JDevlieghere as a code owner November 26, 2024 18:04

llvmbot added the lldb label Nov 26, 2024

walter-erquinigo reviewed Nov 26, 2024

View reviewed changes

clayborg requested review from jimingham and clayborg November 26, 2024 19:15

clayborg reviewed Nov 26, 2024

View reviewed changes

Michael137 mentioned this pull request Nov 28, 2024

[lldb] Update dwim-print to show expanded objc instances #117500

Merged

Bugfix: Not showing the synthetic children of values behind pointers #117755

Are you sure you want to change the base?

Bugfix: Not showing the synthetic children of values behind pointers #117755

Uh oh!

Conversation

skuznetsov commented Nov 26, 2024

Uh oh!

github-actions bot commented Nov 26, 2024

Uh oh!

llvmbot commented Nov 26, 2024

Uh oh!

walter-erquinigo Nov 26, 2024

Choose a reason for hiding this comment

Uh oh!

walter-erquinigo Nov 26, 2024

Choose a reason for hiding this comment

Uh oh!

skuznetsov Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

skuznetsov commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clayborg commented Dec 2, 2024

Uh oh!

clayborg commented Dec 2, 2024

Uh oh!

clayborg commented Dec 2, 2024

Uh oh!

skuznetsov commented Dec 2, 2024

Uh oh!

skuznetsov commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clayborg commented Dec 7, 2024

Uh oh!

clayborg commented Dec 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clayborg commented Dec 7, 2024

Uh oh!

clayborg commented Dec 7, 2024

Uh oh!

skuznetsov commented Dec 8, 2024

Uh oh!

clayborg commented Dec 10, 2024

Uh oh!

skuznetsov commented Feb 1, 2025

Uh oh!

Uh oh!

skuznetsov Nov 26, 2024 •

edited

Loading

skuznetsov commented Nov 26, 2024 •

edited

Loading

skuznetsov commented Dec 2, 2024 •

edited

Loading

clayborg commented Dec 7, 2024 •

edited

Loading