Skip to content

[lldb] Upgrade CompilerType::GetBitSize to return llvm::Expected #129601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 5, 2025

Conversation

adrian-prantl
Copy link
Collaborator

This patch pushes the error handling boundary for the GetBitSize() methods from Runtime into the Type and CompilerType APIs. This makes it easier to diagnose problems thanks to more meaningful error messages being available. GetBitSize() is often the first thing LLDB asks about a type, so this method is particularly important for a better user experience.

rdar://145667239

@adrian-prantl adrian-prantl requested review from labath, Michael137 and JDevlieghere and removed request for JDevlieghere and labath March 3, 2025 22:53
@llvmbot llvmbot added the lldb label Mar 3, 2025
@adrian-prantl adrian-prantl requested a review from labath March 3, 2025 22:53
@llvmbot
Copy link
Member

llvmbot commented Mar 3, 2025

@llvm/pr-subscribers-lldb

Author: Adrian Prantl (adrian-prantl)

Changes

This patch pushes the error handling boundary for the GetBitSize() methods from Runtime into the Type and CompilerType APIs. This makes it easier to diagnose problems thanks to more meaningful error messages being available. GetBitSize() is often the first thing LLDB asks about a type, so this method is particularly important for a better user experience.

rdar://145667239


Patch is 120.40 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/129601.diff

76 Files Affected:

  • (modified) lldb/include/lldb/Expression/ExpressionVariable.h (+1-1)
  • (modified) lldb/include/lldb/Symbol/CompilerType.h (+2-2)
  • (modified) lldb/include/lldb/Symbol/Type.h (+1-1)
  • (modified) lldb/include/lldb/Symbol/TypeSystem.h (+1-1)
  • (modified) lldb/include/lldb/Target/StackFrameRecognizer.h (+1-1)
  • (modified) lldb/include/lldb/ValueObject/ValueObject.h (+1-1)
  • (modified) lldb/include/lldb/ValueObject/ValueObjectCast.h (+1-1)
  • (modified) lldb/include/lldb/ValueObject/ValueObjectChild.h (+1-1)
  • (modified) lldb/include/lldb/ValueObject/ValueObjectConstResult.h (+1-1)
  • (modified) lldb/include/lldb/ValueObject/ValueObjectDynamicValue.h (+1-1)
  • (modified) lldb/include/lldb/ValueObject/ValueObjectMemory.h (+1-1)
  • (modified) lldb/include/lldb/ValueObject/ValueObjectRegister.h (+2-2)
  • (modified) lldb/include/lldb/ValueObject/ValueObjectSyntheticFilter.h (+1-1)
  • (modified) lldb/include/lldb/ValueObject/ValueObjectVTable.h (+1-1)
  • (modified) lldb/include/lldb/ValueObject/ValueObjectVariable.h (+1-1)
  • (modified) lldb/source/API/SBType.cpp (+2-2)
  • (modified) lldb/source/API/SBValue.cpp (+1-1)
  • (modified) lldb/source/Commands/CommandObjectMemory.cpp (+13-12)
  • (modified) lldb/source/Commands/CommandObjectWatchpoint.cpp (+6-4)
  • (modified) lldb/source/Core/Value.cpp (+13-4)
  • (modified) lldb/source/DataFormatters/TypeFormat.cpp (+6-3)
  • (modified) lldb/source/DataFormatters/VectorType.cpp (+9-8)
  • (modified) lldb/source/Expression/ExpressionVariable.cpp (+2-1)
  • (modified) lldb/source/Expression/Materializer.cpp (+46-30)
  • (modified) lldb/source/Plugins/ABI/AArch64/ABIMacOSX_arm64.cpp (+8-6)
  • (modified) lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp (+8-6)
  • (modified) lldb/source/Plugins/ABI/ARM/ABIMacOSX_arm.cpp (+5-3)
  • (modified) lldb/source/Plugins/ABI/ARM/ABISysV_arm.cpp (+9-5)
  • (modified) lldb/source/Plugins/ABI/LoongArch/ABISysV_loongarch.cpp (+2-1)
  • (modified) lldb/source/Plugins/ABI/Mips/ABISysV_mips.cpp (+2-1)
  • (modified) lldb/source/Plugins/ABI/Mips/ABISysV_mips64.cpp (+5-4)
  • (modified) lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp (+9-7)
  • (modified) lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc64.cpp (+8-5)
  • (modified) lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp (+2-1)
  • (modified) lldb/source/Plugins/ABI/SystemZ/ABISysV_s390x.cpp (+5-4)
  • (modified) lldb/source/Plugins/ABI/X86/ABIMacOSX_i386.cpp (+4-2)
  • (modified) lldb/source/Plugins/ABI/X86/ABISysV_i386.cpp (+4-3)
  • (modified) lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp (+9-7)
  • (modified) lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp (+9-7)
  • (modified) lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp (+9-6)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp (+4-2)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/GenericBitset.cpp (+4-4)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp (+2-2)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxxInitializerList.cpp (+5-2)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxxProxyArray.cpp (+4-2)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxxSliceArray.cpp (+2-1)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxxSpan.cpp (+5-2)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxxValarray.cpp (+2-1)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxxVariant.cpp (+7-4)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp (+7-3)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibStdcpp.cpp (+2-1)
  • (modified) lldb/source/Plugins/Language/CPlusPlus/LibStdcppUniquePointer.cpp (+2-1)
  • (modified) lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCClassDescriptorV2.cpp (+1-1)
  • (modified) lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp (+2-1)
  • (modified) lldb/source/Plugins/SymbolFile/CTF/SymbolFileCTF.cpp (+6-8)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+18-12)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+8-5)
  • (modified) lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp (+13-11)
  • (modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp (+46-43)
  • (modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h (+11-9)
  • (modified) lldb/source/Symbol/CompilerType.cpp (+32-22)
  • (modified) lldb/source/Symbol/Type.cpp (+12-10)
  • (modified) lldb/source/Target/StackFrame.cpp (+12-5)
  • (modified) lldb/source/Target/Thread.cpp (+6-3)
  • (modified) lldb/source/ValueObject/ValueObject.cpp (+30-22)
  • (modified) lldb/source/ValueObject/ValueObjectCast.cpp (+1-1)
  • (modified) lldb/source/ValueObject/ValueObjectConstResult.cpp (+9-5)
  • (modified) lldb/source/ValueObject/ValueObjectDynamicValue.cpp (+3-2)
  • (modified) lldb/source/ValueObject/ValueObjectMemory.cpp (+7-3)
  • (modified) lldb/source/ValueObject/ValueObjectRegister.cpp (+2-2)
  • (modified) lldb/source/ValueObject/ValueObjectSyntheticFilter.cpp (+1-1)
  • (modified) lldb/source/ValueObject/ValueObjectVTable.cpp (+3-3)
  • (modified) lldb/source/ValueObject/ValueObjectVariable.cpp (+1-5)
  • (modified) lldb/test/Shell/SymbolFile/DWARF/x86/class-type-nullptr-deref.s (+1-1)
  • (modified) lldb/test/Shell/SymbolFile/DWARF/x86/debug-types-signature-loop.s (+1-1)
  • (modified) lldb/unittests/Platform/PlatformSiginfoTest.cpp (+2-1)
diff --git a/lldb/include/lldb/Expression/ExpressionVariable.h b/lldb/include/lldb/Expression/ExpressionVariable.h
index f5bd938921966..68fa1c878a0e3 100644
--- a/lldb/include/lldb/Expression/ExpressionVariable.h
+++ b/lldb/include/lldb/Expression/ExpressionVariable.h
@@ -33,7 +33,7 @@ class ExpressionVariable
 
   virtual ~ExpressionVariable() = default;
 
-  std::optional<uint64_t> GetByteSize() { return m_frozen_sp->GetByteSize(); }
+  llvm::Expected<uint64_t> GetByteSize() { return m_frozen_sp->GetByteSize(); }
 
   ConstString GetName() { return m_frozen_sp->GetName(); }
 
diff --git a/lldb/include/lldb/Symbol/CompilerType.h b/lldb/include/lldb/Symbol/CompilerType.h
index fe4fcbccee370..41a1676dabd76 100644
--- a/lldb/include/lldb/Symbol/CompilerType.h
+++ b/lldb/include/lldb/Symbol/CompilerType.h
@@ -391,9 +391,9 @@ class CompilerType {
   struct IntegralTemplateArgument;
 
   /// Return the size of the type in bytes.
-  std::optional<uint64_t> GetByteSize(ExecutionContextScope *exe_scope) const;
+  llvm::Expected<uint64_t> GetByteSize(ExecutionContextScope *exe_scope) const;
   /// Return the size of the type in bits.
-  std::optional<uint64_t> GetBitSize(ExecutionContextScope *exe_scope) const;
+  llvm::Expected<uint64_t> GetBitSize(ExecutionContextScope *exe_scope) const;
 
   lldb::Encoding GetEncoding(uint64_t &count) const;
 
diff --git a/lldb/include/lldb/Symbol/Type.h b/lldb/include/lldb/Symbol/Type.h
index 91188fe6ea483..e657357b942f1 100644
--- a/lldb/include/lldb/Symbol/Type.h
+++ b/lldb/include/lldb/Symbol/Type.h
@@ -480,7 +480,7 @@ class Type : public std::enable_shared_from_this<Type>, public UserID {
 
   ConstString GetBaseName();
 
-  std::optional<uint64_t> GetByteSize(ExecutionContextScope *exe_scope);
+  llvm::Expected<uint64_t> GetByteSize(ExecutionContextScope *exe_scope);
 
   llvm::Expected<uint32_t> GetNumChildren(bool omit_empty_base_classes);
 
diff --git a/lldb/include/lldb/Symbol/TypeSystem.h b/lldb/include/lldb/Symbol/TypeSystem.h
index b06bfa583bad6..59fb066e087d3 100644
--- a/lldb/include/lldb/Symbol/TypeSystem.h
+++ b/lldb/include/lldb/Symbol/TypeSystem.h
@@ -312,7 +312,7 @@ class TypeSystem : public PluginInterface,
 
   virtual const llvm::fltSemantics &GetFloatTypeSemantics(size_t byte_size) = 0;
 
-  virtual std::optional<uint64_t>
+  virtual llvm::Expected<uint64_t>
   GetBitSize(lldb::opaque_compiler_type_t type,
              ExecutionContextScope *exe_scope) = 0;
 
diff --git a/lldb/include/lldb/Target/StackFrameRecognizer.h b/lldb/include/lldb/Target/StackFrameRecognizer.h
index 8cf69a82eb8c0..eb603242e2c5e 100644
--- a/lldb/include/lldb/Target/StackFrameRecognizer.h
+++ b/lldb/include/lldb/Target/StackFrameRecognizer.h
@@ -180,7 +180,7 @@ class ValueObjectRecognizerSynthesizedValue : public ValueObject {
     SetName(parent.GetName());
   }
 
-  std::optional<uint64_t> GetByteSize() override {
+  llvm::Expected<uint64_t> GetByteSize() override {
     return m_parent->GetByteSize();
   }
   lldb::ValueType GetValueType() const override { return m_type; }
diff --git a/lldb/include/lldb/ValueObject/ValueObject.h b/lldb/include/lldb/ValueObject/ValueObject.h
index a0f53d20327cd..06d2589002ed0 100644
--- a/lldb/include/lldb/ValueObject/ValueObject.h
+++ b/lldb/include/lldb/ValueObject/ValueObject.h
@@ -357,7 +357,7 @@ class ValueObject {
   virtual bool CanProvideValue();
 
   // Subclasses must implement the functions below.
-  virtual std::optional<uint64_t> GetByteSize() = 0;
+  virtual llvm::Expected<uint64_t> GetByteSize() = 0;
 
   virtual lldb::ValueType GetValueType() const = 0;
 
diff --git a/lldb/include/lldb/ValueObject/ValueObjectCast.h b/lldb/include/lldb/ValueObject/ValueObjectCast.h
index 9d174ae5ca609..e62c33a549a58 100644
--- a/lldb/include/lldb/ValueObject/ValueObjectCast.h
+++ b/lldb/include/lldb/ValueObject/ValueObjectCast.h
@@ -30,7 +30,7 @@ class ValueObjectCast : public ValueObject {
   static lldb::ValueObjectSP Create(ValueObject &parent, ConstString name,
                                     const CompilerType &cast_type);
 
-  std::optional<uint64_t> GetByteSize() override;
+  llvm::Expected<uint64_t> GetByteSize() override;
 
   llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
 
diff --git a/lldb/include/lldb/ValueObject/ValueObjectChild.h b/lldb/include/lldb/ValueObject/ValueObjectChild.h
index e8c974a3a10a7..a386f726b9dda 100644
--- a/lldb/include/lldb/ValueObject/ValueObjectChild.h
+++ b/lldb/include/lldb/ValueObject/ValueObjectChild.h
@@ -29,7 +29,7 @@ class ValueObjectChild : public ValueObject {
 public:
   ~ValueObjectChild() override;
 
-  std::optional<uint64_t> GetByteSize() override { return m_byte_size; }
+  llvm::Expected<uint64_t> GetByteSize() override { return m_byte_size; }
 
   lldb::offset_t GetByteOffset() override { return m_byte_offset; }
 
diff --git a/lldb/include/lldb/ValueObject/ValueObjectConstResult.h b/lldb/include/lldb/ValueObject/ValueObjectConstResult.h
index e4ed1f399bf6b..2ee531f5858e1 100644
--- a/lldb/include/lldb/ValueObject/ValueObjectConstResult.h
+++ b/lldb/include/lldb/ValueObject/ValueObjectConstResult.h
@@ -64,7 +64,7 @@ class ValueObjectConstResult : public ValueObject {
   static lldb::ValueObjectSP Create(ExecutionContextScope *exe_scope,
                                     Status &&error);
 
-  std::optional<uint64_t> GetByteSize() override;
+  llvm::Expected<uint64_t> GetByteSize() override;
 
   lldb::ValueType GetValueType() const override;
 
diff --git a/lldb/include/lldb/ValueObject/ValueObjectDynamicValue.h b/lldb/include/lldb/ValueObject/ValueObjectDynamicValue.h
index 145a46e295566..51a73b8062c22 100644
--- a/lldb/include/lldb/ValueObject/ValueObjectDynamicValue.h
+++ b/lldb/include/lldb/ValueObject/ValueObjectDynamicValue.h
@@ -35,7 +35,7 @@ class ValueObjectDynamicValue : public ValueObject {
 public:
   ~ValueObjectDynamicValue() override = default;
 
-  std::optional<uint64_t> GetByteSize() override;
+  llvm::Expected<uint64_t> GetByteSize() override;
 
   ConstString GetTypeName() override;
 
diff --git a/lldb/include/lldb/ValueObject/ValueObjectMemory.h b/lldb/include/lldb/ValueObject/ValueObjectMemory.h
index cfc36d0d610db..c1bd28434e324 100644
--- a/lldb/include/lldb/ValueObject/ValueObjectMemory.h
+++ b/lldb/include/lldb/ValueObject/ValueObjectMemory.h
@@ -41,7 +41,7 @@ class ValueObjectMemory : public ValueObject {
                                     const Address &address,
                                     const CompilerType &ast_type);
 
-  std::optional<uint64_t> GetByteSize() override;
+  llvm::Expected<uint64_t> GetByteSize() override;
 
   ConstString GetTypeName() override;
 
diff --git a/lldb/include/lldb/ValueObject/ValueObjectRegister.h b/lldb/include/lldb/ValueObject/ValueObjectRegister.h
index fafbfd0341115..0812dc575aaa1 100644
--- a/lldb/include/lldb/ValueObject/ValueObjectRegister.h
+++ b/lldb/include/lldb/ValueObject/ValueObjectRegister.h
@@ -37,7 +37,7 @@ class ValueObjectRegisterSet : public ValueObject {
                                     lldb::RegisterContextSP &reg_ctx_sp,
                                     uint32_t set_idx);
 
-  std::optional<uint64_t> GetByteSize() override;
+  llvm::Expected<uint64_t> GetByteSize() override;
 
   lldb::ValueType GetValueType() const override {
     return lldb::eValueTypeRegisterSet;
@@ -89,7 +89,7 @@ class ValueObjectRegister : public ValueObject {
                                     lldb::RegisterContextSP &reg_ctx_sp,
                                     const RegisterInfo *reg_info);
 
-  std::optional<uint64_t> GetByteSize() override;
+  llvm ::Expected<uint64_t> GetByteSize() override;
 
   lldb::ValueType GetValueType() const override {
     return lldb::eValueTypeRegister;
diff --git a/lldb/include/lldb/ValueObject/ValueObjectSyntheticFilter.h b/lldb/include/lldb/ValueObject/ValueObjectSyntheticFilter.h
index 2811658fd8f1f..df205a258a997 100644
--- a/lldb/include/lldb/ValueObject/ValueObjectSyntheticFilter.h
+++ b/lldb/include/lldb/ValueObject/ValueObjectSyntheticFilter.h
@@ -37,7 +37,7 @@ class ValueObjectSynthetic : public ValueObject {
 public:
   ~ValueObjectSynthetic() override;
 
-  std::optional<uint64_t> GetByteSize() override;
+  llvm::Expected<uint64_t> GetByteSize() override;
 
   ConstString GetTypeName() override;
 
diff --git a/lldb/include/lldb/ValueObject/ValueObjectVTable.h b/lldb/include/lldb/ValueObject/ValueObjectVTable.h
index 9cf13be093a8d..618456cbd120c 100644
--- a/lldb/include/lldb/ValueObject/ValueObjectVTable.h
+++ b/lldb/include/lldb/ValueObject/ValueObjectVTable.h
@@ -62,7 +62,7 @@ class ValueObjectVTable : public ValueObject {
 
   static lldb::ValueObjectSP Create(ValueObject &parent);
 
-  std::optional<uint64_t> GetByteSize() override;
+  llvm::Expected<uint64_t> GetByteSize() override;
 
   llvm::Expected<uint32_t> CalculateNumChildren(uint32_t max) override;
 
diff --git a/lldb/include/lldb/ValueObject/ValueObjectVariable.h b/lldb/include/lldb/ValueObject/ValueObjectVariable.h
index 9f66af808425a..16030cd4edbac 100644
--- a/lldb/include/lldb/ValueObject/ValueObjectVariable.h
+++ b/lldb/include/lldb/ValueObject/ValueObjectVariable.h
@@ -38,7 +38,7 @@ class ValueObjectVariable : public ValueObject {
   static lldb::ValueObjectSP Create(ExecutionContextScope *exe_scope,
                                     const lldb::VariableSP &var_sp);
 
-  std::optional<uint64_t> GetByteSize() override;
+  llvm::Expected<uint64_t> GetByteSize() override;
 
   ConstString GetTypeName() override;
 
diff --git a/lldb/source/API/SBType.cpp b/lldb/source/API/SBType.cpp
index 9eb1f0c75ea05..00f28717d97f3 100644
--- a/lldb/source/API/SBType.cpp
+++ b/lldb/source/API/SBType.cpp
@@ -127,8 +127,8 @@ uint64_t SBType::GetByteSize() {
   LLDB_INSTRUMENT_VA(this);
 
   if (IsValid())
-    if (std::optional<uint64_t> size =
-            m_opaque_sp->GetCompilerType(false).GetByteSize(nullptr))
+    if (std::optional<uint64_t> size = llvm::expectedToOptional(
+            m_opaque_sp->GetCompilerType(false).GetByteSize(nullptr)))
       return *size;
   return 0;
 }
diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
index a707b9aa7589c..6b91120f6427a 100644
--- a/lldb/source/API/SBValue.cpp
+++ b/lldb/source/API/SBValue.cpp
@@ -329,7 +329,7 @@ size_t SBValue::GetByteSize() {
   ValueLocker locker;
   lldb::ValueObjectSP value_sp(GetSP(locker));
   if (value_sp) {
-    result = value_sp->GetByteSize().value_or(0);
+    result = llvm::expectedToOptional(value_sp->GetByteSize()).value_or(0);
   }
 
   return result;
diff --git a/lldb/source/Commands/CommandObjectMemory.cpp b/lldb/source/Commands/CommandObjectMemory.cpp
index f2fa5391f8f33..7140333bb3cde 100644
--- a/lldb/source/Commands/CommandObjectMemory.cpp
+++ b/lldb/source/Commands/CommandObjectMemory.cpp
@@ -519,14 +519,14 @@ class CommandObjectMemoryRead : public CommandObjectParsed {
         --pointer_count;
       }
 
-      std::optional<uint64_t> size = compiler_type.GetByteSize(nullptr);
-      if (!size) {
+      auto size_or_err = compiler_type.GetByteSize(nullptr);
+      if (!size_or_err) {
         result.AppendErrorWithFormat(
-            "unable to get the byte size of the type '%s'\n",
-            view_as_type_cstr);
+            "unable to get the byte size of the type '%s'\n%s",
+            view_as_type_cstr, llvm::toString(size_or_err.takeError()).c_str());
         return;
       }
-      m_format_options.GetByteSizeValue() = *size;
+      m_format_options.GetByteSizeValue() = *size_or_err;
 
       if (!m_format_options.GetCountValue().OptionWasSet())
         m_format_options.GetCountValue() = 1;
@@ -639,15 +639,16 @@ class CommandObjectMemoryRead : public CommandObjectParsed {
       if (!m_format_options.GetFormatValue().OptionWasSet())
         m_format_options.GetFormatValue().SetCurrentValue(eFormatDefault);
 
-      std::optional<uint64_t> size = compiler_type.GetByteSize(nullptr);
-      if (!size) {
-        result.AppendError("can't get size of type");
+      auto size_or_err = compiler_type.GetByteSize(nullptr);
+      if (!size_or_err) {
+        result.AppendError(llvm::toString(size_or_err.takeError()));
         return;
       }
-      bytes_read = *size * m_format_options.GetCountValue().GetCurrentValue();
+      auto size = *size_or_err;
+      bytes_read = size * m_format_options.GetCountValue().GetCurrentValue();
 
       if (argc > 0)
-        addr = addr + (*size * m_memory_options.m_offset.GetCurrentValue());
+        addr = addr + (size * m_memory_options.m_offset.GetCurrentValue());
     } else if (m_format_options.GetFormatValue().GetCurrentValue() !=
                eFormatCString) {
       data_sp = std::make_shared<DataBufferHeap>(total_byte_size, '\0');
@@ -1034,8 +1035,8 @@ class CommandObjectMemoryFind : public CommandObjectParsed {
                frame, result_sp)) &&
           result_sp) {
         uint64_t value = result_sp->GetValueAsUnsigned(0);
-        std::optional<uint64_t> size =
-            result_sp->GetCompilerType().GetByteSize(nullptr);
+        std::optional<uint64_t> size = llvm::expectedToOptional(
+            result_sp->GetCompilerType().GetByteSize(nullptr));
         if (!size)
           return;
         switch (*size) {
diff --git a/lldb/source/Commands/CommandObjectWatchpoint.cpp b/lldb/source/Commands/CommandObjectWatchpoint.cpp
index 766d650a2ca07..20f4b91f15340 100644
--- a/lldb/source/Commands/CommandObjectWatchpoint.cpp
+++ b/lldb/source/Commands/CommandObjectWatchpoint.cpp
@@ -867,9 +867,10 @@ corresponding to the byte size of the data type.");
       if (addr_type == eAddressTypeLoad) {
         // We're in business.
         // Find out the size of this variable.
-        size = m_option_watchpoint.watch_size.GetCurrentValue() == 0
-                   ? valobj_sp->GetByteSize().value_or(0)
-                   : m_option_watchpoint.watch_size.GetCurrentValue();
+        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 {
@@ -1080,7 +1081,8 @@ class CommandObjectWatchpointSetExpression : public CommandObjectRaw {
     /// of the expression, so convert to that if we found a valid type.
     CompilerType compiler_type(valobj_sp->GetCompilerType());
 
-    std::optional<uint64_t> valobj_size = valobj_sp->GetByteSize();
+    std::optional<uint64_t> valobj_size =
+        llvm::expectedToOptional(valobj_sp->GetByteSize());
     // Set the type as a uint8_t array if the size being watched is
     // larger than the ValueObject's size (which is probably the size
     // of a pointer).
diff --git a/lldb/source/Core/Value.cpp b/lldb/source/Core/Value.cpp
index 70299cb8455a1..c91b3f852f986 100644
--- a/lldb/source/Core/Value.cpp
+++ b/lldb/source/Core/Value.cpp
@@ -24,6 +24,8 @@
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/LLDBLog.h"
+#include "lldb/Utility/Log.h"
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/lldb-defines.h"
@@ -223,10 +225,16 @@ uint64_t Value::GetValueByteSize(Status *error_ptr, ExecutionContext *exe_ctx) {
   case ContextType::Variable: // Variable *
   {
     auto *scope = exe_ctx ? exe_ctx->GetBestExecutionContextScope() : nullptr;
-    if (std::optional<uint64_t> size = GetCompilerType().GetByteSize(scope)) {
+    auto size_or_err = GetCompilerType().GetByteSize(scope);
+    if (!size_or_err) {
+      if (error_ptr && error_ptr->Success())
+        *error_ptr = Status::FromError(size_or_err.takeError());
+      else
+        LLDB_LOG_ERRORV(GetLog(LLDBLog::Types), size_or_err.takeError(), "{0}");
+    } else {
       if (error_ptr)
         error_ptr->Clear();
-      return *size;
+      return *size_or_err;
     }
     break;
   }
@@ -321,8 +329,9 @@ Status Value::GetValueAsData(ExecutionContext *exe_ctx, DataExtractor &data,
   AddressType address_type = eAddressTypeFile;
   Address file_so_addr;
   const CompilerType &ast_type = GetCompilerType();
-  std::optional<uint64_t> type_size = ast_type.GetByteSize(
-      exe_ctx ? exe_ctx->GetBestExecutionContextScope() : nullptr);
+  std::optional<uint64_t> type_size =
+      llvm::expectedToOptional(ast_type.GetByteSize(
+          exe_ctx ? exe_ctx->GetBestExecutionContextScope() : nullptr));
   // Nothing to be done for a zero-sized type.
   if (type_size && *type_size == 0)
     return error;
diff --git a/lldb/source/DataFormatters/TypeFormat.cpp b/lldb/source/DataFormatters/TypeFormat.cpp
index 409c452110bdd..456b83938f1ae 100644
--- a/lldb/source/DataFormatters/TypeFormat.cpp
+++ b/lldb/source/DataFormatters/TypeFormat.cpp
@@ -96,16 +96,19 @@ bool TypeFormatImpl_Format::FormatObject(ValueObject *valobj,
 
         ExecutionContextScope *exe_scope =
             exe_ctx.GetBestExecutionContextScope();
-        std::optional<uint64_t> size = compiler_type.GetByteSize(exe_scope);
-        if (!size)
+        auto size_or_err = compiler_type.GetByteSize(exe_scope);
+        if (!size_or_err) {
+          LLDB_LOG_ERRORV(GetLog(LLDBLog::Types), size_or_err.takeError(),
+                          "{0}");
           return false;
+        }
         StreamString sstr;
         compiler_type.DumpTypeValue(
             &sstr,                          // The stream to use for display
             GetFormat(),                    // Format to display this type with
             data,                           // Data to extract from
             0,                              // Byte offset into "m_data"
-            *size,                          // Byte size of item in "m_data"
+            *size_or_err,                   // Byte size of item in "m_data"
             valobj->GetBitfieldBitSize(),   // Bitfield bit size
             valobj->GetBitfieldBitOffset(), // Bitfield bit offset
             exe_scope);
diff --git a/lldb/source/DataFormatters/VectorType.cpp b/lldb/source/DataFormatters/VectorType.cpp
index fa3fb1b674efb..162b075ec87d2 100644
--- a/lldb/source/DataFormatters/VectorType.cpp
+++ b/lldb/source/DataFormatters/VectorType.cpp
@@ -197,15 +197,15 @@ static lldb::Format GetItemFormatForFormat(lldb::Format format,
 static std::optional<size_t>
 CalculateNumChildren(CompilerType container_elem_type, uint64_t num_elements,
                      CompilerType element_type) {
-  std::optional<uint64_t> container_elem_size =
-      container_elem_type.GetByteSize(/* exe_scope */ nullptr);
+  std::optional<uint64_t> container_elem_size = llvm::expectedToOptional(
+      container_elem_type.GetByteSize(/* exe_scope */ nullptr));
   if (!container_elem_size)
     return {};
 
   auto container_size = *container_elem_size * num_elements;
 
-  std::optional<uint64_t> element_size =
-      element_type.GetByteSize(/* exe_scope */ nullptr);
+  std::optional<uint64_t> element_size = llvm::expectedToOptional(
+      element_type.GetByteSize(/* exe_scope */ nullptr));
   if (!element_size || !*element_size)
     return {};
 
@@ -236,10 +236,11 @@ class VectorTypeSyntheticFrontEnd : public SyntheticChildrenFrontEnd {
           nullptr, Status::FromError(num_children_or_err.takeError()));
     if (idx >= *num_children_or_err)
       return {};
-    std::optional<uint64_t> size = m_child_type.GetByteSize(nullptr);
-    if (!size)
-      return {};
-    auto offset = idx * *size;
+    auto size_or_err = m_child_type.GetByteSize(nullptr);
+    if (!size_or_err)
+      return ValueObjectConstResult::Create(
+          nullptr, Status::FromError(size_or_err.takeError()));
+    auto offset = idx * *size_or_err;
     StreamString idx_name;
     idx_name.Printf("[%" PRIu64 "]", (uint64_t)idx);
     ValueObjectSP child_sp(m_backend.GetSyntheticChildAtOffset(
diff --git a/lldb/source/Expression/ExpressionVariable.cpp b/lldb/source/Expression/ExpressionVariable.cpp
index f0a28988822fa..9e8ea60f8e052 100644
--- a/lldb/source/Expression/ExpressionVariable.cpp
+++ b/lldb/source/Expression/ExpressionVariable.cpp
@@ -20,7 +20,8 @@ char ExpressionVariable::ID;
 ExpressionVariable::ExpressionVariable() : m_flags(0) {}
 
 uint8_t *ExpressionVariable::GetValueBytes() {
-  std::optional<uint64_t> byte_size = m_frozen_sp->GetByteSize();
+  std::optional<uint64_t> byte_size =
+      llvm::e...
[truncated]

Copy link
Member

@Michael137 Michael137 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Left some minor nits

@@ -101,7 +101,8 @@ lldb::ChildCacheState LibStdcppUniquePtrSyntheticFrontEnd::Update() {
// storage due to no_unique_address, so infer the actual size from the total
// size of the unique_ptr class. If sizeof(unique_ptr) == sizeof(void*) then
// the deleter is empty and should be hidden.
if (tuple_sp->GetByteSize() > ptr_obj->GetByteSize()) {
if (llvm::expectedToOptional(tuple_sp->GetByteSize()).value_or(0) >
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here we invent a new default .value_or(0), whereas we used to compare the optionals directly. Previously if we had 0 > std::nullopt, this condition would still fire, but with the new defaults it wouldn't. There are a couple other places where this changed.

Probably not something that happens in practice, and possibly not even what the original code intended, but wanted to point that out to make sure we're not accidentally changing something subtle that we relied on

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this, and I think all of these cases were subtle potential regressions I introduced when I changed the return type from uint64_t to optional<uint64_t> a year(?) ago. So this new form is actually closer to the original code.

@adrian-prantl adrian-prantl force-pushed the 145667239 branch 2 times, most recently from 83f6e65 to f69ac50 Compare March 5, 2025 01:34
Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about llvm::expectedToOptional. LGTM!

This patch pushes the error handling boundary for the GetBitSize()
methods from Runtime into the Type and CompilerType APIs. This makes
it easier to diagnose problems thanks to more meaningful error
messages being available. GetBitSize() is often the first thing LLDB
asks about a type, so this method is particularly important for
a better user experience.

rdar://145667239
@adrian-prantl adrian-prantl merged commit 878a64f into llvm:main Mar 5, 2025
10 checks passed
adrian-prantl added a commit to adrian-prantl/llvm-project that referenced this pull request Mar 9, 2025
…m#129601)

This patch pushes the error handling boundary for the GetBitSize()
methods from Runtime into the Type and CompilerType APIs. This makes
it easier to diagnose problems thanks to more meaningful error
messages being available. GetBitSize() is often the first thing LLDB
asks about a type, so this method is particularly important for a
better user experience.

rdar://145667239
(cherry picked from commit 878a64f)
adrian-prantl added a commit to adrian-prantl/llvm-project that referenced this pull request Mar 13, 2025
…m#129601)

This patch pushes the error handling boundary for the GetBitSize()
methods from Runtime into the Type and CompilerType APIs. This makes
it easier to diagnose problems thanks to more meaningful error
messages being available. GetBitSize() is often the first thing LLDB
asks about a type, so this method is particularly important for a
better user experience.

rdar://145667239
(cherry picked from commit 878a64f)
jph-13 pushed a commit to jph-13/llvm-project that referenced this pull request Mar 21, 2025
…m#129601)

This patch pushes the error handling boundary for the GetBitSize()
methods from Runtime into the Type and CompilerType APIs. This makes it
easier to diagnose problems thanks to more meaningful error messages
being available. GetBitSize() is often the first thing LLDB asks about a
type, so this method is particularly important for a better user
experience.

rdar://145667239
AnthonyLatsis pushed a commit to AnthonyLatsis/llvm-project that referenced this pull request May 1, 2025
…e to return llvm::Expected (llvm#129601)"

This patch pushes the error handling boundary for the GetBitSize()
methods from Runtime into the Type and CompilerType APIs. This makes
it easier to diagnose problems thanks to more meaningful error
messages being available. GetBitSize() is often the first thing LLDB
asks about a type, so this method is particularly important for a
better user experience.

rdar://145667239
(cherry picked from commit 878a64f)

(cherry picked from commit 642dcd7)
AnthonyLatsis pushed a commit to AnthonyLatsis/llvm-project that referenced this pull request May 1, 2025
…e to return llvm::Expected (llvm#129601)"

This patch pushes the error handling boundary for the GetBitSize()
methods from Runtime into the Type and CompilerType APIs. This makes
it easier to diagnose problems thanks to more meaningful error
messages being available. GetBitSize() is often the first thing LLDB
asks about a type, so this method is particularly important for a
better user experience.

rdar://145667239
(cherry picked from commit 878a64f)

(cherry picked from commit 642dcd7)
bnbarham pushed a commit to swiftlang/llvm-project that referenced this pull request May 5, 2025
…e to return llvm::Expected (llvm#129601)"

This patch pushes the error handling boundary for the GetBitSize()
methods from Runtime into the Type and CompilerType APIs. This makes
it easier to diagnose problems thanks to more meaningful error
messages being available. GetBitSize() is often the first thing LLDB
asks about a type, so this method is particularly important for a
better user experience.

rdar://145667239
(cherry picked from commit 878a64f)

(cherry picked from commit 642dcd7)
(cherry picked from commit d3e7a33)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants