[LLDB] Fix ValueObject::AddressOf() return value

[kuilpd]

ValueObject::AddressOf() used to return address as a value which has it's own address, allowing to do value.AddressOf().AddressOf().
This patch makes the return address a simple const value.

[github-actions]

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

[kuilpd]

If I apply code formatter to the test, it breaks lines and no longer works. Should I rewrite it without using inline tests?

[jimingham]

What is the reason for making this change?

[kuilpd]

What is the reason for making this change?

The result value of AddressOf() shouldn't have its own address, it should be like an r-value const, and in the current implementation it just has the same address as the value it was taken from.

(lldb) script lldb.frame.FindVariable("y")
(int) y = 47
(lldb) script lldb.frame.FindVariable("y").AddressOf()
(int *) &y = 0x00007fffffffd7d0
(lldb) script lldb.frame.FindVariable("y").AddressOf().AddressOf()
(int **) &&y = 0x00007fffffffd7d0
(lldb) script lldb.frame.FindVariable("y").AddressOf().AddressOf().AddressOf()
No value

[jimingham]

This is at least consistent with what we do for ValueObjectConstResult::AddressOf

[labath]

If I apply code formatter to the test, it breaks lines and no longer works. Should I rewrite it without using inline tests?

You can stick a // clang-format off before the block. We don't tend to write inline tests these days, but we haven't really deprecated them either, and this looks like a good place to add the additional check.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Ilia Kuklin (kuilpd)

Changes

ValueObject::AddressOf() used to return address as a value which has it's own address, allowing to do value.AddressOf().AddressOf().
This patch makes the return address a simple const value.

---

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

2 Files Affected:

• (modified) lldb/source/ValueObject/ValueObject.cpp (+5-2)
• (modified) lldb/test/API/python_api/sbvalue_const_addrof/main.cpp (+25-22)

diff --git a/lldb/source/ValueObject/ValueObject.cpp b/lldb/source/ValueObject/ValueObject.cpp
index 8741cb7343166..1d8d112ccb0f4 100644
--- a/lldb/source/ValueObject/ValueObject.cpp
+++ b/lldb/source/ValueObject/ValueObject.cpp
@@ -2966,10 +2966,13 @@ ValueObjectSP ValueObject::AddressOf(Status &error) {
         std::string name(1, '&');
         name.append(m_name.AsCString(""));
         ExecutionContext exe_ctx(GetExecutionContextRef());
+
+        lldb::DataBufferSP buffer(
+            new lldb_private::DataBufferHeap(&addr, sizeof(lldb::addr_t)));
         m_addr_of_valobj_sp = ValueObjectConstResult::Create(
             exe_ctx.GetBestExecutionContextScope(),
-            compiler_type.GetPointerType(), ConstString(name.c_str()), addr,
-            eAddressTypeInvalid, m_data.GetAddressByteSize());
+            compiler_type.GetPointerType(), ConstString(name.c_str()), buffer,
+            endian::InlHostByteOrder(), exe_ctx.GetAddressByteSize());
       }
     } break;
     default:
diff --git a/lldb/test/API/python_api/sbvalue_const_addrof/main.cpp b/lldb/test/API/python_api/sbvalue_const_addrof/main.cpp
index 318a45bc21a85..ae6abc8613406 100644
--- a/lldb/test/API/python_api/sbvalue_const_addrof/main.cpp
+++ b/lldb/test/API/python_api/sbvalue_const_addrof/main.cpp
@@ -3,21 +3,21 @@
 
 struct RegisterContext
 {
-    uintptr_t r0;
-    uintptr_t r1;
-    uintptr_t r2;
-    uintptr_t r3;
-    uintptr_t r4;
-    uintptr_t pc;
-    uintptr_t fp;
-    uintptr_t sp;
+  uintptr_t r0;
+  uintptr_t r1;
+  uintptr_t r2;
+  uintptr_t r3;
+  uintptr_t r4;
+  uintptr_t pc;
+  uintptr_t fp;
+  uintptr_t sp;
 };
 
 struct ThreadInfo {
-    uint32_t tid;
-    const char *name;
-    RegisterContext regs;
-    ThreadInfo *next;
+  uint32_t tid;
+  const char *name;
+  RegisterContext regs;
+  ThreadInfo *next;
 };
 int main (int argc, char const *argv[], char const *envp[]);
 
@@ -27,14 +27,17 @@ ThreadInfo *g_thread_list_ptr = &g_thread1;
 
 int main (int argc, char const *argv[], char const *envp[])
 {
-    printf ("g_thread_list is %p\n", g_thread_list_ptr);
-    return 0; //% v = self.dbg.GetSelectedTarget().FindFirstGlobalVariable('g_thread_list_ptr')
-    //% v_gla = v.GetChildMemberWithName('regs').GetLoadAddress()
-    //% v_aof = v.GetChildMemberWithName('regs').AddressOf().GetValueAsUnsigned(lldb.LLDB_INVALID_ADDRESS)
-    //% expr = '(%s)0x%x' % (v.GetType().GetName(), v.GetValueAsUnsigned(0))
-    //% e = v.CreateValueFromExpression('e', expr)
-    //% e_gla = e.GetChildMemberWithName('regs').GetLoadAddress()
-    //% e_aof = e.GetChildMemberWithName('regs').AddressOf().GetValueAsUnsigned(lldb.LLDB_INVALID_ADDRESS)
-    //% self.assertTrue(v_gla == e_gla, "GetLoadAddress() differs")
-    //% self.assertTrue(v_aof == e_aof, "AddressOf() differs")
+  // clang-format off
+  printf ("g_thread_list is %p\n", g_thread_list_ptr);
+  return 0; //% v = self.dbg.GetSelectedTarget().FindFirstGlobalVariable('g_thread_list_ptr')
+  //% self.assertTrue(v.AddressOf().IsValid())
+  //% self.assertFalse(v.AddressOf().AddressOf().IsValid())
+  //% v_gla = v.GetChildMemberWithName('regs').GetLoadAddress()
+  //% v_aof = v.GetChildMemberWithName('regs').AddressOf().GetValueAsUnsigned(lldb.LLDB_INVALID_ADDRESS)
+  //% expr = '(%s)0x%x' % (v.GetType().GetName(), v.GetValueAsUnsigned(0))
+  //% e = v.CreateValueFromExpression('e', expr)
+  //% e_gla = e.GetChildMemberWithName('regs').GetLoadAddress()
+  //% e_aof = e.GetChildMemberWithName('regs').AddressOf().GetValueAsUnsigned(lldb.LLDB_INVALID_ADDRESS)
+  //% self.assertTrue(v_gla == e_gla, "GetLoadAddress() differs")
+  //% self.assertTrue(v_aof == e_aof, "AddressOf() differs")
 }

[kuilpd]

@labath
Is this okay to merge now?

[labath]

Yeah, yeah. If Jim says it's ok, that's good enough for me.