[lldb] Refactor away UB in SBValue::GetLoadAddress (llvm#141799)

The problem was in calling GetLoadAddress on a value in the error state,
where `ValueObject::GetLoadAddress` could end up accessing the
uninitialized "address type" by-ref return value from `GetAddressOf`.
This probably happened because each function expected the other to
initialize it.

We can guarantee initialization by turning this into a proper return
value.

I've added a test, but it only (reliably) crashes if lldb is built with
ubsan.

Author: labath

lldb/include/lldb/ValueObject/ValueObject.h

@@ -573,10 +573,14 @@ class ValueObject {
   /// child as well.
   void SetName(ConstString name) { m_name = name; }
 
-  virtual lldb::addr_t GetAddressOf(bool scalar_is_load_address = true,
-                                    AddressType *address_type = nullptr);
+  struct AddrAndType {
+    lldb::addr_t address = LLDB_INVALID_ADDRESS;
+    AddressType type = eAddressTypeInvalid;
+  };
+
+  virtual AddrAndType GetAddressOf(bool scalar_is_load_address = true);
 
-  lldb::addr_t GetPointerValue(AddressType *address_type = nullptr);
+  AddrAndType GetPointerValue();
 
   lldb::ValueObjectSP GetSyntheticChild(ConstString key) const;
 

lldb/include/lldb/ValueObject/ValueObjectConstResult.h

@@ -86,8 +86,7 @@ class ValueObjectConstResult : public ValueObject {
 
   lldb::ValueObjectSP AddressOf(Status &error) override;
 
-  lldb::addr_t GetAddressOf(bool scalar_is_load_address = true,
-                            AddressType *address_type = nullptr) override;
+  AddrAndType GetAddressOf(bool scalar_is_load_address = true) override;
 
   size_t GetPointeeData(DataExtractor &data, uint32_t item_idx = 0,
                         uint32_t item_count = 1) override;

lldb/include/lldb/ValueObject/ValueObjectConstResultChild.h

@@ -48,8 +48,7 @@ class ValueObjectConstResultChild : public ValueObjectChild {
 
   lldb::ValueObjectSP AddressOf(Status &error) override;
 
-  lldb::addr_t GetAddressOf(bool scalar_is_load_address = true,
-                            AddressType *address_type = nullptr) override;
+  AddrAndType GetAddressOf(bool scalar_is_load_address = true) override;
 
   size_t GetPointeeData(DataExtractor &data, uint32_t item_idx = 0,
                         uint32_t item_count = 1) override;

lldb/include/lldb/ValueObject/ValueObjectConstResultImpl.h

@@ -10,6 +10,7 @@
 #define LLDB_VALUEOBJECT_VALUEOBJECTCONSTRESULTIMPL_H
 
 #include "lldb/Utility/ConstString.h"
+#include "lldb/ValueObject/ValueObject.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-private-enumerations.h"
@@ -21,7 +22,6 @@ namespace lldb_private {
 class CompilerType;
 class DataExtractor;
 class Status;
-class ValueObject;
 } // namespace lldb_private
 
 namespace lldb_private {
@@ -58,8 +58,8 @@ class ValueObjectConstResultImpl {
     m_live_address_type = address_type;
   }
 
-  virtual lldb::addr_t GetAddressOf(bool scalar_is_load_address = true,
-                                    AddressType *address_type = nullptr);
+  virtual ValueObject::AddrAndType
+  GetAddressOf(bool scalar_is_load_address = true);
 
   virtual size_t GetPointeeData(DataExtractor &data, uint32_t item_idx = 0,
                                 uint32_t item_count = 1);

lldb/source/API/SBValue.cpp

@@ -1336,10 +1336,8 @@ lldb::SBAddress SBValue::GetAddress() {
   if (value_sp) {
     TargetSP target_sp(value_sp->GetTargetSP());
     if (target_sp) {
-      lldb::addr_t value = LLDB_INVALID_ADDRESS;
-      const bool scalar_is_load_address = true;
-      AddressType addr_type;
-      value = value_sp->GetAddressOf(scalar_is_load_address, &addr_type);
+      auto [value, addr_type] =
+          value_sp->GetAddressOf(/*scalar_is_load_address=*/true);
       if (addr_type == eAddressTypeFile) {
         ModuleSP module_sp(value_sp->GetModule());
         if (module_sp)

lldb/source/Commands/CommandObjectWatchpoint.cpp

@@ -822,7 +822,6 @@ corresponding to the byte size of the data type.");
 
     // We passed the sanity check for the command. Proceed to set the
     // watchpoint now.
-    lldb::addr_t addr = 0;
     size_t size = 0;
 
     VariableSP var_sp;
@@ -861,19 +860,7 @@ corresponding to the byte size of the data type.");
 
     CompilerType compiler_type;
 
-    if (valobj_sp) {
-      AddressType addr_type;
-      addr = valobj_sp->GetAddressOf(false, &addr_type);
-      if (addr_type == eAddressTypeLoad) {
-        // We're in business.
-        // Find out the size of this variable.
-        size =
-            m_option_watchpoint.watch_size.GetCurrentValue() == 0
-                ? llvm::expectedToOptional(valobj_sp->GetByteSize()).value_or(0)
-                : m_option_watchpoint.watch_size.GetCurrentValue();
-      }
-      compiler_type = valobj_sp->GetCompilerType();
-    } else {
+    if (!valobj_sp) {
       const char *error_cstr = error.AsCString(nullptr);
       if (error_cstr)
         result.AppendError(error_cstr);
@@ -883,6 +870,16 @@ corresponding to the byte size of the data type.");
                                      command.GetArgumentAtIndex(0));
       return;
     }
+    auto [addr, addr_type] = valobj_sp->GetAddressOf(false);
+    if (addr_type == eAddressTypeLoad) {
+      // We're in business.
+      // Find out the size of this variable.
+      size =
+          m_option_watchpoint.watch_size.GetCurrentValue() == 0
+              ? llvm::expectedToOptional(valobj_sp->GetByteSize()).value_or(0)
+              : m_option_watchpoint.watch_size.GetCurrentValue();
+    }
+    compiler_type = valobj_sp->GetCompilerType();
 
     // Now it's time to create the watchpoint.
     uint32_t watch_type = 0;

lldb/source/DataFormatters/CXXFunctionPointer.cpp

@@ -24,8 +24,7 @@ using namespace lldb_private::formatters;
 bool lldb_private::formatters::CXXFunctionPointerSummaryProvider(
     ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {
   StreamString sstr;
-  AddressType func_ptr_address_type = eAddressTypeInvalid;
-  addr_t func_ptr_address = valobj.GetPointerValue(&func_ptr_address_type);
+  auto [func_ptr_address, func_ptr_address_type] = valobj.GetPointerValue();
   if (func_ptr_address != 0 && func_ptr_address != LLDB_INVALID_ADDRESS) {
     switch (func_ptr_address_type) {
     case eAddressTypeInvalid:

lldb/source/DataFormatters/FormattersHelpers.cpp

@@ -113,15 +113,16 @@ lldb_private::formatters::ExtractIndexFromString(const char *item_name) {
 
 Address
 lldb_private::formatters::GetArrayAddressOrPointerValue(ValueObject &valobj) {
-  lldb::addr_t data_addr = LLDB_INVALID_ADDRESS;
-  AddressType type;
+  ValueObject::AddrAndType data_addr;
 
   if (valobj.IsPointerType())
-    data_addr = valobj.GetPointerValue(&type);
+    data_addr = valobj.GetPointerValue();
   else if (valobj.IsArrayType())
-    data_addr = valobj.GetAddressOf(/*scalar_is_load_address=*/true, &type);
-  if (data_addr != LLDB_INVALID_ADDRESS && type == eAddressTypeFile)
-    return Address(data_addr, valobj.GetModule()->GetSectionList());
+    data_addr = valobj.GetAddressOf(/*scalar_is_load_address=*/true);
 
-  return data_addr;
+  if (data_addr.address != LLDB_INVALID_ADDRESS &&
+      data_addr.type == eAddressTypeFile)
+    return Address(data_addr.address, valobj.GetModule()->GetSectionList());
+
+  return data_addr.address;
 }

lldb/source/DataFormatters/TypeFormat.cpp

@@ -80,7 +80,7 @@ bool TypeFormatImpl_Format::FormatObject(ValueObject *valobj,
               Status error;
               WritableDataBufferSP buffer_sp(
                   new DataBufferHeap(max_len + 1, 0));
-              Address address(valobj->GetPointerValue());
+              Address address(valobj->GetPointerValue().address);
               target_sp->ReadCStringFromMemory(
                   address, (char *)buffer_sp->GetBytes(), max_len, error);
               if (error.Success())

lldb/source/DataFormatters/ValueObjectPrinter.cpp

@@ -542,8 +542,7 @@ bool ValueObjectPrinter::ShouldPrintChildren(
   if (is_ptr || is_ref) {
     // We have a pointer or reference whose value is an address. Make sure
     // that address is not NULL
-    AddressType ptr_address_type;
-    if (valobj.GetPointerValue(&ptr_address_type) == 0)
+    if (valobj.GetPointerValue().address == 0)
       return false;
 
     const bool is_root_level = m_curr_depth == 0;