Make Type::GetByteSize optional

[aprantl]

Summary

This is a continuation of my quest to make the size 0 a supported value.

[labath]

Looks fine to me.

I guess this would be the place I'd start thinking about an OptionalSize class (to replace the two fields in the Type class). But I don't think that's necessary yet...

[clayborg]

include/lldb/Symbol/Type.h

221

Does this need to be uint64_t to share the same space as m_byte_size?

[aprantl]

include/lldb/Symbol/Type.h

221

I don't think so, but it might be more readable.

[aprantl]

Pavel, I may need you help. I reverted the commit because it broke three PDB-related tests (SymbolFilePDBTests, func-symbols, and typedefs) that need more than just lld in order to run. The fix is probably trivial. We only need to set a breakpoint in Type.cpp:119 and every time a type with size 0 is passed in look at the frame we're coming from and make sure that that llvm::None is passed in instead of 0, similar to the changes in DWARFASTParserClang.cpp.

[labath]

The attached patch is what it took to get the tests passing for me. It could use a bit of cleanup though.

size.diff1 KBDownload

[aprantl]

Thanks a bunch, Pavel! That is very helpful.

[stella.stamenova]

I am not sure if you saw this, but it looks like it broke one of the tests on the Windows bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/1092

include/lldb/Symbol/Type.h

public:
   typedef enum ResolveStateTag : unsigned {
     eResolveStateUnresolved = 0,
     eResolveStateForward = 1,
     eResolveStateLayout = 2,
     eResolveStateFull = 3
   } ResolveState;
 
   Type(lldb::user_id_t uid, SymbolFile *symbol_file, const ConstString &name,
-       uint64_t byte_size, SymbolContextScope *context,
+       llvm::Optional<uint64_t> byte_size, SymbolContextScope *context,
        lldb::user_id_t encoding_uid, EncodingDataType encoding_uid_type,
        const Declaration &decl, const CompilerType &compiler_qual_type,
        ResolveState compiler_type_resolve_state);
 
   // This makes an invalid type.  Used for functions that return a Type when
   // they get an error.
   Type();
 
public:
 
   SymbolFile *GetSymbolFile() { return m_symbol_file; }
   const SymbolFile *GetSymbolFile() const { return m_symbol_file; }
 
   TypeList *GetTypeList();
 
   const ConstString &GetName();
 
-  uint64_t GetByteSize();
+  llvm::Optional<uint64_t> GetByteSize();
 
   uint32_t GetNumChildren(bool omit_empty_base_classes);
 
   bool IsAggregateType();
 
   bool IsValidType() { return m_encoding_uid_type != eEncodingInvalid; }
 
   bool IsTypedef() { return m_encoding_uid_type == eEncodingIsTypedefUID; }

 protected:
   ConstString m_name;
   SymbolFile *m_symbol_file;
   SymbolContextScope
       *m_context; // The symbol context in which this type is defined
   Type *m_encoding_type;
   lldb::user_id_t m_encoding_uid;
   EncodingDataType m_encoding_uid_type;
-  uint64_t m_byte_size;
+  uint64_t m_byte_size : 63;
+  unsigned m_byte_size_has_value : 1;
   Declaration m_decl;
   CompilerType m_compiler_type;
 
   struct Flags {
 #ifdef __GNUC__
     // using unsigned type here to work around a very noisy gcc warning
     unsigned compiler_type_resolve_state : 2;
 #else

source/Core/ValueObjectMemory.cpp

size_t ValueObjectMemory::CalculateNumChildren(uint32_t max) {
   const bool omit_empty_base_classes = true;
   auto child_count =
       m_compiler_type.GetNumChildren(omit_empty_base_classes, &exe_ctx);
   return child_count <= max ? child_count : max;
 }
 
 uint64_t ValueObjectMemory::GetByteSize() {
   if (m_type_sp)
-    return m_type_sp->GetByteSize();
+    if (llvm::Optional<uint64_t> size = m_type_sp->GetByteSize())
+      return *size;
   if (llvm::Optional<uint64_t> size = m_compiler_type.GetByteSize(nullptr))
     return *size;
   return 0;
 }
 
 lldb::ValueType ValueObjectMemory::GetValueType() const {
   // RETHINK: Should this be inherited from somewhere?
   return lldb::eValueTypeVariableGlobal;

source/Expression/Materializer.cpp

if (m_is_reference) {
             err.SetErrorStringWithFormat("the variable '%s' has no location, "
                                          "it may have been optimized out",
                                          m_variable_sp->GetName().AsCString());
           } else {
             err.SetErrorStringWithFormat(
                 "size of variable %s (%" PRIu64
                 ") is larger than the ValueObject's size (%" PRIu64 ")",
                 m_variable_sp->GetName().AsCString(),
-                m_variable_sp->GetType()->GetByteSize(), data.GetByteSize());
+                m_variable_sp->GetType()->GetByteSize().getValueOr(0),
+                data.GetByteSize());
           }
           return;
         }
 
         size_t bit_align =
             m_variable_sp->GetType()->GetLayoutCompilerType().GetTypeBitAlign();
         size_t byte_align = (bit_align + 7) / 8;
 

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

if (type_ptr == NULL) {
               clang_type, Type::eResolveStateForward));
 
           if (ClangASTContext::StartTagDeclarationDefinition(clang_type)) {
             if (die.HasChildren()) {
               SymbolContext cu_sc(die.GetLLDBCompileUnit());
               bool is_signed = false;
               enumerator_clang_type.IsIntegerType(is_signed);
               ParseChildEnumerators(cu_sc, clang_type, is_signed,
-                                    type_sp->GetByteSize(), die);
+                                    type_sp->GetByteSize().getValueOr(0), die);
             }
             ClangASTContext::CompleteTagDeclarationDefinition(clang_type);
           } else {
             dwarf->GetObjectFile()->GetModule()->ReportError(
                 "DWARF DIE at 0x%8.8x named \"%s\" was not able to start its "
                 "definition.\nPlease file a bug and attach the file at the "
                 "start of this error message",
                 die.GetOffset(), type_name_cstr);

           if (element_type) {
             auto array_info = ParseChildArrayInfo(die);
             if (array_info) {
               first_index = array_info->first_index;
               byte_stride = array_info->byte_stride;
               bit_stride = array_info->bit_stride;
             }
             if (byte_stride == 0 && bit_stride == 0)
-              byte_stride = element_type->GetByteSize();
+              byte_stride = element_type->GetByteSize().getValueOr(0);
             CompilerType array_element_type =
                 element_type->GetForwardCompilerType();
 
             if (ClangASTContext::IsCXXClassType(array_element_type) &&
                 !array_element_type.GetCompleteType()) {
               ModuleSP module_sp = die.GetModule();
               if (module_sp) {
                 if (die.GetCU()->GetProducer() == eProducerClang)
case DW_TAG_class_type: {
     m_ast.AddMethodOverridesForCXXRecordType(clang_type.GetOpaqueQualType());
     ClangASTContext::BuildIndirectFields(clang_type);
     ClangASTContext::CompleteTagDeclarationDefinition(clang_type);
 
     if (!layout_info.field_offsets.empty() ||
         !layout_info.base_offsets.empty() ||
         !layout_info.vbase_offsets.empty()) {
       if (type)
-        layout_info.bit_size = type->GetByteSize() * 8;
+        layout_info.bit_size = type->GetByteSize().getValueOr(0) * 8;
       if (layout_info.bit_size == 0)
         layout_info.bit_size =
             die.GetAttributeValueAsUnsigned(DW_AT_byte_size, 0) * 8;
 
       clang::CXXRecordDecl *record_decl =
           m_ast.GetAsCXXRecordDecl(clang_type.GetOpaqueQualType());
       if (record_decl) {
         if (log) {
case DW_TAG_class_type: {
     return (bool)clang_type;
 
   case DW_TAG_enumeration_type:
     if (ClangASTContext::StartTagDeclarationDefinition(clang_type)) {
       if (die.HasChildren()) {
         SymbolContext sc(die.GetLLDBCompileUnit());
         bool is_signed = false;
         clang_type.IsIntegerType(is_signed);
-        ParseChildEnumerators(sc, clang_type, is_signed, type->GetByteSize(),
-                              die);
+        ParseChildEnumerators(sc, clang_type, is_signed,
+                              type->GetByteSize().getValueOr(0), die);
       }
       ClangASTContext::CompleteTagDeclarationDefinition(clang_type);
     }
     return (bool)clang_type;
 
   default:
     assert(false && "not a forward clang type decl!");
     break;
case DW_TAG_APPLE_property: {
                 //
                 // AT_bit_size indicates the size of the field in bits.
                 /////////////////////////////////////////////////////////////
 
                 if (data_bit_offset != UINT64_MAX) {
                   this_field_info.bit_offset = data_bit_offset;
                 } else {
                   if (byte_size == 0)
-                    byte_size = member_type->GetByteSize();
+                    byte_size = member_type->GetByteSize().getValueOr(0);
 
                   ObjectFile *objfile = die.GetDWARF()->GetObjectFile();
                   if (objfile->GetByteOrder() == eByteOrderLittle) {
                     this_field_info.bit_offset += byte_size * 8;
                     this_field_info.bit_offset -= (bit_offset + bit_size);
                   } else {
                     this_field_info.bit_offset += bit_offset;
                   }

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

if (module_sp) {
                 if (location.Evaluate(nullptr, LLDB_INVALID_ADDRESS, nullptr,
                                       nullptr, location_result, &error)) {
                   if (location_result.GetValueType() ==
                       Value::eValueTypeFileAddress) {
                     lldb::addr_t file_addr =
                         location_result.GetScalar().ULongLong();
                     lldb::addr_t byte_size = 1;
                     if (var_sp->GetType())
-                      byte_size = var_sp->GetType()->GetByteSize();
+                      byte_size =
+                          var_sp->GetType()->GetByteSize().getValueOr(0);
                     m_global_aranges_ap->Append(GlobalVariableMap::Entry(
                         file_addr, byte_size, var_sp.get()));
                   }
                 }
               }
             }
           }
         }
if (num_attributes > 0) {
         }
       }
 
       if (symbol_context_scope) {
         SymbolFileTypeSP type_sp(
             new SymbolFileType(*this, DIERef(type_die_form).GetUID(this)));
 
         if (const_value.Form() && type_sp && type_sp->GetType())
-          location.CopyOpcodeData(const_value.Unsigned(),
-                                  type_sp->GetType()->GetByteSize(),
-                                  die.GetCU()->GetAddressByteSize());
+          location.CopyOpcodeData(
+              const_value.Unsigned(),
+              type_sp->GetType()->GetByteSize().getValueOr(0),
 
         var_sp.reset(new Variable(die.GetID(), name, mangled, type_sp, scope,
                                   symbol_context_scope, scope_ranges, &decl,
                                   location, is_external, is_artificial,
                                   is_static_member));
 
         var_sp->SetLocationIsConstantValueData(location_is_const_value_data);
       } else {

source/Symbol/Type.cpp

if (!m_type_sp) {
     Type *resolved_type = m_symbol_file.ResolveTypeUID(GetID());
     if (resolved_type)
       m_type_sp = resolved_type->shared_from_this();
   }
   return m_type_sp.get();
 }
 
 Type::Type(lldb::user_id_t uid, SymbolFile *symbol_file,
-           const ConstString &name, uint64_t byte_size,
+           const ConstString &name, llvm::Optional<uint64_t> byte_size,
            SymbolContextScope *context, user_id_t encoding_uid,
            EncodingDataType encoding_uid_type, const Declaration &decl,
            const CompilerType &compiler_type,
            ResolveState compiler_type_resolve_state)
     : std::enable_shared_from_this<Type>(), UserID(uid), m_name(name),
       m_symbol_file(symbol_file), m_context(context), m_encoding_type(nullptr),
       m_encoding_uid(encoding_uid), m_encoding_uid_type(encoding_uid_type),
-      m_byte_size(byte_size), m_decl(decl), m_compiler_type(compiler_type) {
+      m_decl(decl), m_compiler_type(compiler_type) {
+  if (byte_size) {
+    m_byte_size = *byte_size;
+    m_byte_size_has_value = true;
+  } else {
+    m_byte_size = 0;
+    m_byte_size_has_value = false;
+  }
   m_flags.compiler_type_resolve_state =
       (compiler_type ? compiler_type_resolve_state : eResolveStateUnresolved);
   m_flags.is_complete_objc_class = false;
 }
 
 Type::Type()
     : std::enable_shared_from_this<Type>(), UserID(0), m_name("<INVALID TYPE>"),
       m_symbol_file(nullptr), m_context(nullptr), m_encoding_type(nullptr),
       m_encoding_uid(LLDB_INVALID_UID), m_encoding_uid_type(eEncodingInvalid),
-      m_byte_size(0), m_decl(), m_compiler_type() {
+      m_byte_size(0), m_byte_size_has_value(false), m_decl(),
+      m_compiler_type() {
   m_flags.compiler_type_resolve_state = eResolveStateUnresolved;
   m_flags.is_complete_objc_class = false;
 }
 
 Type::Type(const Type &rhs)
     : std::enable_shared_from_this<Type>(rhs), UserID(rhs), m_name(rhs.m_name),
       m_symbol_file(rhs.m_symbol_file), m_context(rhs.m_context),
       m_encoding_type(rhs.m_encoding_type), m_encoding_uid(rhs.m_encoding_uid),
       m_encoding_uid_type(rhs.m_encoding_uid_type),
-      m_byte_size(rhs.m_byte_size), m_decl(rhs.m_decl),
+      m_byte_size(rhs.m_byte_size),
+      m_byte_size_has_value(rhs.m_byte_size_has_value), m_decl(rhs.m_decl),
       m_compiler_type(rhs.m_compiler_type), m_flags(rhs.m_flags) {}
 
 const Type &Type::operator=(const Type &rhs) {
   if (this != &rhs) {
   }
   return *this;
 }
 

 
 void Type::Dump(Stream *s, bool show_context) {
   s->Printf("%p: ", static_cast<void *>(this));
   s->Indent();
   *s << "Type" << static_cast<const UserID &>(*this) << ' ';
   if (m_name)
     *s << ", name = \"" << m_name << "\"";
 
-  if (m_byte_size != 0)
+  if (m_byte_size_has_value)
     s->Printf(", size = %" PRIu64, m_byte_size);
 
   if (show_context && m_context != nullptr) {
     s->PutCString(", context = ( ");
     m_context->DumpSymbolContext(s);
     s->PutCString(" )");
   }
 
if (show_types) {
       if (verbose)
         s->Printf("Type{0x%8.8" PRIx64 "} ", GetID());
       DumpTypeName(s);
       s->PutCString(") ");
     }
 
     GetForwardCompilerType().DumpValue(
         exe_ctx, s, format == lldb::eFormatDefault ? GetFormat() : format, data,
-        data_byte_offset, GetByteSize(),
+        data_byte_offset, GetByteSize().getValueOr(0),
         0, // Bitfield bit size
         0, // Bitfield bit offset
         show_types, show_summary, verbose, 0);
   }
 }
 
 Type *Type::GetEncodingType() {
   if (m_encoding_type == nullptr && m_encoding_uid != LLDB_INVALID_UID)
     m_encoding_type = m_symbol_file->ResolveTypeUID(m_encoding_uid);
   return m_encoding_type;
 }
 
-uint64_t Type::GetByteSize() {
-  if (m_byte_size == 0) {
-    switch (m_encoding_uid_type) {
-    case eEncodingInvalid:
-    case eEncodingIsSyntheticUID:
-      break;
-    case eEncodingIsUID:
-    case eEncodingIsConstUID:
-    case eEncodingIsRestrictUID:
-    case eEncodingIsVolatileUID:
-    case eEncodingIsTypedefUID: {
-      Type *encoding_type = GetEncodingType();
-      if (encoding_type)
-        m_byte_size = encoding_type->GetByteSize();
-      if (m_byte_size == 0)
-        if (llvm::Optional<uint64_t> size =
-                GetLayoutCompilerType().GetByteSize(nullptr))
-          m_byte_size = *size;
+llvm::Optional<uint64_t> Type::GetByteSize() {
+  if (m_byte_size_has_value)
+    return m_byte_size;
+
+      if (llvm::Optional<uint64_t> size = encoding_type->GetByteSize()) {
+        m_byte_size = *size;
+        m_byte_size_has_value = true;
+      }
+    
+    if (!m_byte_size_has_value)
+          GetLayoutCompilerType().GetByteSize(nullptr)) {
+        m_byte_size_has_value = true;
+      }
     } break;
 
     // If we are a pointer or reference, then this is just a pointer size;
     case eEncodingIsPointerUID:
     case eEncodingIsLValueReferenceUID:
     case eEncodingIsRValueReferenceUID: {
-      if (ArchSpec arch = m_symbol_file->GetObjectFile()->GetArchitecture())
+      if (ArchSpec arch = m_symbol_file->GetObjectFile()->GetArchitecture()) {
         m_byte_size = arch.GetAddressByteSize();
-    } break;
+        m_byte_size_has_value = true;
       }
+    } break;
   }
-  return m_byte_size;
+  return {};
 }
 
 uint32_t Type::GetNumChildren(bool omit_empty_base_classes) {
   return GetForwardCompilerType().GetNumChildren(omit_empty_base_classes, nullptr);
 }
 
 bool Type::IsAggregateType() {
   return GetForwardCompilerType().IsAggregateType();

 bool Type::ReadFromMemory(ExecutionContext *exe_ctx, lldb::addr_t addr,
                           AddressType address_type, DataExtractor &data) {
   if (address_type == eAddressTypeFile) {
     // Can't convert a file address to anything valid without more context
     // (which Module it came from)
     return false;
   }
 
-  const uint64_t byte_size = GetByteSize();
+  const uint64_t byte_size = GetByteSize().getValueOr(0);
   if (data.GetByteSize() < byte_size) {
     lldb::DataBufferSP data_sp(new DataBufferHeap(byte_size, '\0'));
     data.SetData(data_sp);
   }
 
   uint8_t *dst = const_cast<uint8_t *>(data.PeekData(0, byte_size));
   if (dst != nullptr) {
     if (address_type == eAddressTypeHost) {