[SymbolFilePDB] Fix null array access when parsing the type of a function without any arguments, i.e. 'int main()' and add support to test it

[asmith]

Summary

• Fix a null array access bug. This happens when creating the lldb type for a function that has no argument.
• Implement SymbolFilePDB::ParseTypes method. Using lldb-test symbols will show all supported types in the target.
• Create lldb types for variadic function, PDBSymbolTypePointer, PDBSymbolTypeBuiltin
• The underlying builtin type for PDBSymbolTypeEnum is always Int, correct it with the very first enumerator's encoding if any. This is more accurate when the underlying type is not signed or another integer type.
• Fix a bug when the compiler type is not created based on PDB_BuiltinType. For example, basic type long is of same width as int in a 32-bit target, and the compiler type of former one will be represented by the one generated for latter if using the default method. Introduce a static function GetBuiltinTypeForPDBEncodingAndBitSize to correct this issue.
• Basic type long double and double have the same bit size in MSVC and there is no information in a PDB to distinguish them. The compiler type of the former one is represented by the latter's.
• There is no line informaton about typedef, enum etc in a PDB and the source and line information for them are not shown.
• There is no information about scoped enumeration. The compiler type is represented as an unscoped one.

[zturner]

This is another example where we could test it easily if lldb-test could dump this. Are you willing to take a stab at this?

[davide]

Indeed, this needs to be tested.

[asmith]

This fix is part of a larger set of changes to retrieve the type for a function signature and I don't see how to test for this without those changes. With all the other changes, lldb-test fails without this fix and passes with it. So it's implicitly already tested.

[zturner]

This was originally written as a unit test because at the time we didn't have lldb-test. To be honest I think it's time to remove these checked in binaries and convert everything to FileCheck tests. There's a couple of reasons this is more practical. For starters, the binaries can be really large and having lots of large binaries in the repo is a real drag for developers. Secondly, we've recently had some bug reports where some virus scan software is flagging our executables as malicious due to some heuristics. So while you're just modifying an existing binary / test, if it's not a ton of work I would really prefer standardizing on lldb-test + FileCheck for this sort of thing. Did you explore that at all?

While there's a little higher up front cost because you have to go update the lldb-test tool to support this, after that work is done it will make writing future tests very easy. All you would need is something like this:

REQUIRES: system-windows
RUN: clang-cl /Z7 /c %S/Inputs/foo.cpp /o %T/foo.cpp.obj
RUN: link /DEBUG /PDB:%T/foo.cpp.pdb /Fo:%T/foo.cpp.exe
RUN: lldb-test symbols -types foo.cpp.exe | FileCheck %s

CHECK: int(*FuncPointerTypedef)();
CHECK: int(*VariadicFuncPointerTypedef)(char,...);

[asmith]

Do you prefer to drop the test changes and corresponding binaries?
For an llvm-lit test, which directory do you recommend for the new file?

[asmith]

The test is added and this is ready to go. Let me know if there are any other changes. Thanks!

[zturner]

Sorry for forgetting about this! lgtm

[asmith]

Revision was approved by zturner in the comments

[majnemer]

lldb/trunk/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp

162–163 ↗

(On Diff #130688)

Copy paste bug?

[labath]

I have reverted this because this breaks the lldb-Unit::SymbolFilePDBTests.TestTypedefs test http://lab.llvm.org:8011/builders/lldb-windows7-android/builds/7715. My guess is that this is because you did not update the checked-in exe to account for the changes in it's source. I'll leave up to you and Zachary to figure out whether you actually want to update the exe or test everything using the lldb-test approach, but in the mean time, we cannot have failing tests in the repository.

lit/SymbolFile/PDB/Inputs/SimpleTypesTest.cpp

 
 typedef float (*VarArgsFuncTypedefA)(...);
 VarArgsFuncTypedefA VarArgsFuncVarA;
 
 // unscoped enum
 enum Enum { RED, GREEN, BLUE };
 Enum EnumVar;
 
-enum EnumConst {LOW, NORMAL = 10, HIGH };
+enum EnumConst { LOW, NORMAL = 10, HIGH };
 EnumConst EnumConstVar;
 
 enum EnumEmpty {};
 EnumEmpty EnumEmptyVar;
 
 enum EnumUChar : unsigned char { ON, OFF, AUTO };
 EnumUChar EnumCharVar;
 

lit/SymbolFile/PDB/enums-layout.test

 REQUIRES: windows
 RUN: clang-cl -m32 /Z7 /c /GS- %S/Inputs/SimpleTypesTest.cpp /o %T/SimpleTypesTest.cpp.enums.obj
-RUN: link %T/SimpleTypesTest.cpp.enums.obj /DEBUG /nodefaultlib /Entry:main /OUT:%T/SimpleTypesTest.cpp.enums.exe
+RUN: link %T/SimpleTypesTest.cpp.enums.obj /DEBUG /nodefaultlib /ENTRY:main /OUT:%T/SimpleTypesTest.cpp.enums.exe
 RUN: lldb-test symbols %T/SimpleTypesTest.cpp.enums.exe | FileCheck %s
 
-; FIXME: For now, PDB does not have information about scoped enumeration (Enum class). 
-; So compiler type of it is represented by the one generated for unscoped enumeration.
+; FIXME: PDB does not have information about scoped enumeration (Enum class) so the  
+; compiler type used is the same as the one for unscoped enumeration.
 
 CHECK: Module [[CU:.*]]
 CHECK-DAG: {{^[0-9A-F]+}}: SymbolVendor ([[CU]])
 CHECK:      Type{{.*}} , name = "Enum", size = 4, compiler_type = {{.*}} enum Enum {
 CHECK-NEXT:    RED,
 CHECK-NEXT:    GREEN,
 CHECK-NEXT:    BLUE
 CHECK-NEXT:}

lit/SymbolFile/PDB/typedefs.test

 REQUIRES: windows
 RUN: clang-cl -m32 /Z7 /c /GS- %S/Inputs/SimpleTypesTest.cpp /o %T/SimpleTypesTest.cpp.typedefs.obj
-RUN: link %T/SimpleTypesTest.cpp.typedefs.obj /DEBUG /nodefaultlib /Entry:main /OUT:%T/SimpleTypesTest.cpp.typedefs.exe
+RUN: link %T/SimpleTypesTest.cpp.typedefs.obj /DEBUG /nodefaultlib /ENTRY:main /OUT:%T/SimpleTypesTest.cpp.typedefs.exe
 RUN: lldb-test symbols %T/SimpleTypesTest.cpp.typedefs.exe | FileCheck %s
 
 ; Generate 32-bit target
 
-; FIXME: For now, PDB does not have line information for typedef statements.
-; So source and line information of them not tested.
+; FIXME: PDB does not have line information for typedef statements so source
+; and line information for them is not tested.
 
-; Note, basic type `long double` and `double` have same bit size in MSVC.
-; There is no information in PDB to distinguish them. So compiler type of them
-; is represented by the one generated for `double`.
+; Note, types `long double` and `double` have same bit size in MSVC and there
+; is no information in the PDB to distinguish them. So the compiler type for 
+; both of them is the same.
 
 CHECK: Module [[CU:.*]]
 CHECK-DAG:  {{^[0-9A-F]+}}: SymbolVendor ([[CU]])
 CHECK:      Type{{.*}} , name = "unsigned long", size = 4, compiler_type = {{.*}} unsigned long
 CHECK-NEXT: Type{{.*}} , size = 40, compiler_type = {{.*}} unsigned long [10]
 CHECK-NEXT: Type{{.*}} , name = "ULongArrayTypedef", compiler_type = {{.*}} typedef ULongArrayTypedef
 
 ; Note: compiler_type of `long double` is represented by the one for `double`

source/Plugins/SymbolFile/PDB/PDBASTParser.cpp

case PDB_BuiltinType::ULong:
     if (width == ast->getTypeSize(ast->UnsignedLongLongTy))
       return CompilerType(ast, ast->UnsignedLongLongTy);
     break;
   case PDB_BuiltinType::WCharT:
     if (width == ast->getTypeSize(ast->WCharTy))
       return CompilerType(ast, ast->WCharTy);
     break;
   case PDB_BuiltinType::Float:
-    // Note: Basic type `long double` and `double` have same bit size in MSVC.
-    // PDB does not have information to distinguish them. So when falling back
-    // to default search, compiler type of `long double` will be represented by
+    // Note: types `long double` and `double` have same bit size in MSVC and there
+    // is no information in the PDB to distinguish them. So when falling back
+    // to default search, the compiler type of `long double` will be represented by
     // the one generated for `double`.
     break;
   }
-  // If not match up PDB_BuiltinType, fall back to default search by encoding
-  // and width only
+  // If there is no match on PDB_BuiltinType, fall back to default search
+  // by encoding and width only
   return clang_ast->GetBuiltinTypeForEncodingAndBitSize(encoding, width);
 }
 
 ConstString GetPDBBuiltinTypeName(const PDBSymbolTypeBuiltin *pdb_type,
                                   CompilerType &compiler_type) {
   if (!pdb_type)
     return compiler_type.GetTypeName();