Fix bug in printing ValueObjects and in PE/COFF Plugin

[zturner]

Summary

The first bug is that the implementation of GetCompilerType for ValueObjectVariable only requests the forward type. This is insufficient for printing enum decls. I suspect this works with the DWARF plugin because the DWARF plugin probably completes enums immediately. The PDB plugin implements enums similarly to other tag records though, and only completes them lazily. ValueObjectVariable then requests the forward compiler type, and makes no attempt to complete it, resulting in insufficient information to print out the value of the enum.

Note that in general, you need a full compiler type to print out the variable anyway. If it's a class type you need it to print out its children, and this already happens when the ValueObject calls GetNumChildren. So this particular change isn't actually invoking any additional work or parsing that wasn't already going to happen anyway, it just allows the case where a symbol file plugin parses enums lazily.

The second bug fixed here is in ObjectFilePECOFF. Both MSVC and clang-cl emit their bss section as a magic section called .data with a raw size of 0 and a file offset of 0. These 3 properties together are sufficient to indicate "this is actually a BSS section", but we had no mechanism of reading from such sections. So we would just try the default read algorithm, which looks at the size, sees that it's too small (0 bytes), and gives up. So we need to add support for this in ObjectFilePECOFF.

This fixes all of the outstanding printing bugs in the native pdb reading tests.

[zturner]

I added a test specifically for the bss bug. Even though one of my existing broken tests became fixed, someone could come down the line and adjust the test and set one of the variables to 1 which would cause the data to go into a regular section. To make sure this doesn't happen, I made a specific test just for this, and used llvm-readobj to also check that that the size of the section is in fact 0. So this test will always fail if the section stops being a compressed bss section for whatever reason.

[lemo]

Shouldn't we also handle the general case where raw size < virtual size? (which would naturally subsume this fix)

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

1073 ↗

(On Diff #173089)

shouldn't this be a hard error instead of returning 0?

1077 ↗

(On Diff #173089)

do we really need to return the available "bytes" if the full read size can't be fullfiled? (ie. shouldn't that be a hard error instead?)

[zturner]

I actually found an even better fix for this. LLDB sections already have the notion of "zero fill sections", we just need to mark the .data section as a zero fill section in this condition. No need to override ReadSectionData

[clayborg]

Very close, just use the layout type as mentioned in the inline comment.

lldb/source/Core/ValueObjectVariable.cpp

70 ↗

(On Diff #173103)

Change this to GetLayoutCompilerType(). Why? If you have just a class Foo and your variable's type is "Foo *", then you don't need to complete it. The layout type gets you the type that is needed in order to display the current variable. For enums this will do the right thing. Otherwise if you have 1000 variables that are all pointers, you will be completing all of the class types for no reason.

[zturner]

Use the layout type instead of the full type, along with a comment explaining why this is necessary.

lit/SymbolFile/NativePDB/Inputs/globals-bss.lldbinit

+target variable GlobalVariable
+
+quit

lit/SymbolFile/NativePDB/ast-reconstruction.cpp

 Anonymous<A::B::C<int>>::D AnonABCVoidD;
 
 // FIXME: Enum size isn't being correctly determined.
 // FIXME: Can't read memory for variable values.
 
 // CHECK: (TrivialC) TC = {}
 // CHECK: (TrivialS) TS = {}
 // CHECK: (TrivialU) TU = {}
-// CHECK: (TrivialE) TE = <Unable to determine byte size.>
-// CHECK: (A::B::C<int>) ABCInt = (ABCMember = <read memory from {{.*}} failed>)
-// CHECK: (A::B::C<float>) ABCFloat = (ABCMember = <read memory from {{.*}} failed>)
-// CHECK: (A::B::C<void>) ABCVoid = (ABCSpecializationMember = <read memory from {{.*}} failed>)
+// CHECK: (TrivialE) TE = TE_A
+// CHECK: (A::B::C<int>) ABCInt = (ABCMember = 0)
+// CHECK: (A::B::C<float>) ABCFloat = (ABCMember = 0)
+// CHECK: (A::B::C<void>) ABCVoid = (ABCSpecializationMember = 0x0000000000000000)
 // CHECK: (A::C<0>) AC0 = {}
 // CHECK: (A::C<-1>) ACNeg1 = {}
-// CHECK: (A::C<0>::D) AC0D = (ACDMember = <read memory from {{.*}} failed>, CPtr = <read memory from {{.*}} failed>)
-// CHECK: (A::C<-1>::D) ACNeg1D = (ACDMember = <read memory from {{.*}} failed>, CPtr = <read memory from {{.*}} failed>)
+// CHECK: (A::C<0>::D) AC0D = (ACDMember = 0, CPtr = 0x0000000000000000)
+// CHECK: (A::C<-1>::D) ACNeg1D = (ACDMember = 0, CPtr = 0x0000000000000000)
 // CHECK: (A::D) AD = {}
-// CHECK: (A::D::E) ADE = (ADDMember = <read memory from {{.*}} failed>)
+// CHECK: (A::D::E) ADE = (ADDMember = 0)
 // CHECK: Dumping clang ast for 1 modules.
 // CHECK: TranslationUnitDecl {{.*}}
 // CHECK: |-CXXRecordDecl {{.*}} class TrivialC definition
 // CHECK: |-CXXRecordDecl {{.*}} struct TrivialS definition
 // CHECK: |-CXXRecordDecl {{.*}} union TrivialU definition
 // CHECK: |-EnumDecl {{.*}} TrivialE
+// CHECK: | `-EnumConstantDecl {{.*}} TE_A 'int'
 // CHECK: |-NamespaceDecl {{.*}} A
 // CHECK: | |-NamespaceDecl {{.*}} B
 // CHECK: | | |-CXXRecordDecl {{.*}} struct C<int> definition
 // CHECK: | | | `-FieldDecl {{.*}} ABCMember 'int'
 // CHECK: | | |-CXXRecordDecl {{.*}} struct C<float> definition
 // CHECK: | | | `-FieldDecl {{.*}} ABCMember 'float'
 // CHECK: | | `-CXXRecordDecl {{.*}} struct C<void> definition
 // CHECK: | |   `-FieldDecl {{.*}} ABCSpecializationMember 'void *'
 // CHECK: | |-CXXRecordDecl {{.*}} struct C<0> definition
 // CHECK: | | `-CXXRecordDecl {{.*}} class D definition
 // CHECK: | |   |-FieldDecl {{.*}} ACDMember 'int'
 // CHECK: | |   `-FieldDecl {{.*}} CPtr 'A::C<-1> *'
 // CHECK: | |-CXXRecordDecl {{.*}} struct C<-1> definition
 // CHECK: | | `-CXXRecordDecl {{.*}} class D definition
 // CHECK: | |   |-FieldDecl {{.*}} ACDMember 'int'
 // CHECK: | |   `-FieldDecl {{.*}} CPtr 'A::C<-2> *'
 // CHECK: | |-CXXRecordDecl {{.*}} struct C<-2>
 // CHECK: | `-CXXRecordDecl {{.*}} struct D definition
 // CHECK: |   `-CXXRecordDecl {{.*}} struct E definition
 // CHECK: |     `-FieldDecl {{.*}} ADDMember 'int'
 
 int main(int argc, char **argv) {
   return 0;
 }
 No newline at end of file

lit/SymbolFile/NativePDB/globals-bss.cpp

+// clang-format off
+// REQUIRES: lld
+
+// Make sure we can read variables from BSS
+// RUN: clang-cl /Z7 /GS- /GR- /c /Fo%t.obj -- %s
+// RUN: lld-link /DEBUG /nodefaultlib /entry:main /OUT:%t.exe /PDB:%t.pdb -- %t.obj
+// RUN: llvm-readobj -s %t.exe | FileCheck --check-prefix=BSS %s
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb -f %t.exe -s \
+// RUN:     %p/Inputs/globals-bss.lldbinit 2>&1 | FileCheck %s
+
+int GlobalVariable = 0;
+
+int main(int argc, char **argv) {
+  return 0;
+}
+
+// BSS:       Section {
+// BSS:         Number: 3
+// BSS:         Name: .data
+// BSS-NEXT:    VirtualSize: 0x4
+// BSS-NEXT:    VirtualAddress:
+// BSS-NEXT:    RawDataSize: 0
+// BSS-NEXT:    PointerToRawData: 0x0
+// BSS-NEXT:    PointerToRelocations: 0x0
+// BSS-NEXT:    PointerToLineNumbers: 0x0
+// BSS-NEXT:    RelocationCount: 0
+// BSS-NEXT:    LineNumberCount: 0
+// BSS-NEXT:    Characteristics [ (0xC0000040)
+// BSS-NEXT:      IMAGE_SCN_CNT_INITIALIZED_DATA (0x40)
+// BSS-NEXT:      IMAGE_SCN_MEM_READ (0x40000000)
+// BSS-NEXT:      IMAGE_SCN_MEM_WRITE (0x80000000)
+// BSS-NEXT:    ]
+// BSS-NEXT:  }
+
+// CHECK: (int) GlobalVariable = 0

source/Core/ValueObjectVariable.cpp

ValueObjectVariable::ValueObjectVariable(ExecutionContextScope *exe_scope,
   assert(m_variable_sp.get() != NULL);
   m_name = var_sp->GetName();
 }
 
 ValueObjectVariable::~ValueObjectVariable() {}
 
 CompilerType ValueObjectVariable::GetCompilerTypeImpl() {
   Type *var_type = m_variable_sp->GetType();
-  if (var_type)
-    return var_type->GetForwardCompilerType();
+  if (!var_type)
     return CompilerType();
+
+  // It's important to return the layout type here.  If we have an enum then the
+  // symbol file plugin may have decided to complete it lazily, in which case a
+  // forward type won't be sufficient to display the variable.  On the other
+  // hand, if we have a pointer to a class type, then getting the full type will
+  // resolve the class type, which is too much.  The layout type is both
+  // necessary and sufficient.
+  return var_type->GetLayoutCompilerType();
 }
 
 ConstString ValueObjectVariable::GetTypeName() {
   Type *var_type = m_variable_sp->GetType();
   if (var_type)
     return var_type->GetName();
   return ConstString();
 }

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

if (module_sp) {
         if (m_sect_headers[idx].flags & llvm::COFF::IMAGE_SCN_CNT_CODE &&
             ((const_sect_name == g_code_sect_name) ||
              (const_sect_name == g_CODE_sect_name))) {
           section_type = eSectionTypeCode;
         } else if (m_sect_headers[idx].flags &
                        llvm::COFF::IMAGE_SCN_CNT_INITIALIZED_DATA &&
                    ((const_sect_name == g_data_sect_name) ||
                     (const_sect_name == g_DATA_sect_name))) {
+          if (m_sect_headers[idx].size == 0 && m_sect_headers[idx].offset == 0)
+            section_type = eSectionTypeZeroFill;
+          else
             section_type = eSectionTypeData;
         } else if (m_sect_headers[idx].flags &
                        llvm::COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA &&
                    ((const_sect_name == g_bss_sect_name) ||
                     (const_sect_name == g_BSS_sect_name))) {
           if (m_sect_headers[idx].size == 0)
             section_type = eSectionTypeZeroFill;
           else
             section_type = eSectionTypeData;
if ((m_coff_header.flags & llvm::COFF::IMAGE_FILE_DLL) == 0)
       return eTypeExecutable;
     else
       return eTypeSharedLibrary;
   }
   return eTypeExecutable;
 }
 
 ObjectFile::Strata ObjectFilePECOFF::CalculateStrata() { return eStrataUser; }
+
 //------------------------------------------------------------------
 // PluginInterface protocol
 //------------------------------------------------------------------
 ConstString ObjectFilePECOFF::GetPluginName() { return GetPluginNameStatic(); }
 
 uint32_t ObjectFilePECOFF::GetPluginVersion() { return 1; }