Page MenuHomePhabricator

[PDB] Parse UDT symbols and pointers to members (combined patch)
ClosedPublic

Authored by aleksandr.urakov on Jul 30 2018, 5:17 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

lit/SymbolFile/PDB/udt-layout.test
1–51 ↗(On Diff #157945)

I've preserved this Windows-only test (but also have included other non-execution tests, which may become cross-platform some later, as Pavel has mentioned at D49410), but if you'll find it unnecessary I'll remove it.

Added support of a MSInheritance attribute.

Ping!

Can you review this, please?

I am OOO this week and only have access to mobile, so hopefully someone
else can review it

Unfortunately, there was no people yet, who can review this :)

Ping! Can anyone review this, please?

Ok I’ll take a look later today then when i get in

zturner accepted this revision.Aug 13 2018, 2:40 PM
This revision is now accepted and ready to land.Aug 13 2018, 2:40 PM
This revision was automatically updated to reflect the committed changes.

@aleksandr.urakov Not sure if you are already working of this, but just FYI this patch introduced a few compiler warnings:

/Users/teemperor/llvm/sidestuff/llvm/tools/lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:55:3: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]       
  default:                                                                                                                                                                                                 
  ^                                                                                                                                                                                                        
/Users/teemperor/llvm/sidestuff/llvm/tools/lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:216:3: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]      
  default:                                                                                                                                                                                                 
  ^                                                                                                                                                                                                        
/Users/teemperor/llvm/sidestuff/llvm/tools/lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp:229:3: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]      
  default:                                                                                                                                                                                                 
  ^                                                                                                                                                                                                        
3 warnings generated.
 20% [125/610] Building CXX object tools/lldb/source/Symbol/CMakeFiles/lldbSymbol.dir/ClangASTContext.cpp.o                                                                                               
/Users/teemperor/llvm/sidestuff/llvm/tools/lldb/source/Symbol/ClangASTContext.cpp:6547:45: warning: comparison of integers of different signs: 'const int32_t' (aka 'const int') and 'unsigned int' [-Wsig$-compare]
                            if (base_offset != UINT32_MAX) {
                                ~~~~~~~~~~~ ^  ~~~~~~~~~~
/Users/teemperor/llvm/sidestuff/llvm/tools/lldb/source/Symbol/ClangASTContext.cpp:6760:34: warning: comparison of integers of different signs: 'int32_t' (aka 'int') and 'unsigned int' [-Wsign-compare]  
                  if (bit_offset == UINT32_MAX)
                      ~~~~~~~~~~ ^  ~~~~~~~~~~
2 warnings generated.

Thanks for catching that!

I didn't know about that warnings because I build LLVM with MSVC compiler, but it doesn't introduce such warnings. Now I've fixed the problem with rLLDB339994.

I am not 100% sure yet, but this change is the most likely culprit for one of the SymbolFile tests to start failing, namely: SymbolFile/DWARF/dwarf5-index-is-used.cpp. This fails for us on both Windows and Linux with the following:

FAIL: lldb :: SymbolFile/DWARF/dwarf5-index-is-used.cpp (42214 of 43490)
******************** TEST 'lldb :: SymbolFile/DWARF/dwarf5-index-is-used.cpp' FAILED ********************
Script:
--
: 'RUN: at line 5';   clang /vstsdrive/_work/11/s/llvm/tools/lldb/lit/SymbolFile/DWARF/dwarf5-index-is-used.cpp -g -c -o /vstsdrive/_work/11/b/LLVMBuild/tools/lldb/lit/SymbolFile/DWARF/Output/dwarf5-index-is-used.cpp.tmp.o --target=x86_64-pc-linux -mllvm -accel-tables=Dwarf
: 'RUN: at line 6';   ld.lld /vstsdrive/_work/11/b/LLVMBuild/tools/lldb/lit/SymbolFile/DWARF/Output/dwarf5-index-is-used.cpp.tmp.o -o /vstsdrive/_work/11/b/LLVMBuild/tools/lldb/lit/SymbolFile/DWARF/Output/dwarf5-index-is-used.cpp.tmp
: 'RUN: at line 7';   /vstsdrive/_work/11/b/LLVMBuild/bin/lldb-test symbols /vstsdrive/_work/11/b/LLVMBuild/tools/lldb/lit/SymbolFile/DWARF/Output/dwarf5-index-is-used.cpp.tmp | /vstsdrive/_work/11/b/LLVMBuild/bin/FileCheck /vstsdrive/_work/11/s/llvm/tools/lldb/lit/SymbolFile/DWARF/dwarf5-index-is-used.cpp
--
Exit Code: 1

Command Output (stderr):
--
/vstsdrive/_work/11/s/llvm/tools/lldb/lit/SymbolFile/DWARF/dwarf5-index-is-used.cpp:9:11: error: CHECK: expected string not found in input
// CHECK: Name Index
          ^
<stdin>:1:1: note: scanning from here
Module: /vstsdrive/_work/11/b/LLVMBuild/tools/lldb/lit/SymbolFile/DWARF/Output/dwarf5-index-is-used.cpp.tmp
^
<stdin>:36:5: note: possible intended match here
IDX name type flags addr offset size link info addralgn entsize Name
    ^

--

********************

I tried looking through the results from the bots to see if the test is passing online and as far as I can tell, none of the bots that are currently online have ran it or if they did, the results got lost because of the failures from the new vscode tests.

Yes, I've seen today that this test is failing now, but if I'm not mistaken this have happened some later than this patch was committed. But I'm not sure, I'll look at this more precisely tomorrow, thanks!

It's also possible that the test has been failing since some commit in another repository (e.g. clang, lld or llvm). It seems that it was ok several days ago...

It's also possible that the test has been failing since some commit in another repository (e.g. clang, lld or llvm). It seems that it was ok several days ago...

I think you are right. I've been trying to isolate when the failure started happening and it appears to be one of the changes made on Aug. 20 in one of the repositories (still trying to narrow that down).

It seems that the cause of the failure is rL340206, but I'm not sure if the adding of -gpubnames switch to clang will be a correct fix for that?