Page MenuHomePhabricator

[LLDB] - Improved DWARF5 support.
ClosedPublic

Authored by grimar on Sep 11 2018, 8:59 AM.

Details

Summary

This patch improves the support of DWARF5 by lldb.

Imagine we have the following code,
(compiled with clang++ test.cpp -g -o test -gdwarf-5):

struct XXX {
  int A;
};

int main() {
  XXX Obj;
  Obj.A = 1;
  return Obj.A;
}

Without this patch when lldb stops on a breakpoint or dump variables
the output is:

(lldb) b main
warning: (x86_64) /home/umb/tests_2018/95_lldb/repro/dwarf5_nosplit/test unsupported DW_FORM value: 0x25
Breakpoint 1: where = test`main, address = 0x0000000000400550
(lldb) run
Process 5589 launched: '/home/umb/tests_2018/95_lldb/repro/dwarf5_nosplit/test' (x86_64)
Process 5589 stopped
* thread #1, name = 'test', stop reason = breakpoint 1.1
    frame #0: 0x0000000000400550 test`main
test`main:
->  0x400550 <+0>:  pushq  %rbp
    0x400551 <+1>:  movq   %rsp, %rbp
    0x400554 <+4>:  movl   $0x0, -0x4(%rbp)
    0x40055b <+11>: movl   $0x1, -0x8(%rbp)
(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> print lldb.frame.GetVariables(True, True, True, True)
<empty> lldb.SBValueList()

Note that it complains about unknown forms,
there is no code source lines and output from the print
call is empty.

With the patch applied output becomes:

umb@umb-virtual-machine:~/LLVM/build_lldb/bin$ ./lldb ~/tests_2018/95_lldb/repro/dwarf5_nosplit/test
(lldb) target create "/home/umb/tests_2018/95_lldb/repro/dwarf5_nosplit/test"
Current executable set to '/home/umb/tests_2018/95_lldb/repro/dwarf5_nosplit/test' (x86_64).
(lldb) b main
Breakpoint 1: where = test`main + 11 at test.cpp:7, address = 0x000000000040055b
(lldb) run
Process 63624 launched: '/home/umb/tests_2018/95_lldb/repro/dwarf5_nosplit/test' (x86_64)
Process 63624 stopped
* thread #1, name = 'test', stop reason = breakpoint 1.1
    frame #0: 0x000000000040055b test`main at test.cpp:7
   4   	
   5   	int main() {
   6   	  XXX Obj;
-> 7   	  Obj.A = 1;
   8   	
   9   	  return Obj.A;
   10  	}
(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> print lldb.frame.GetVariables(True, True, True, True)
(XXX) Obj = (A = 0)

Note there is no test case yet, I am going to add it to this revision soon.
This is my first patch for lldb and I did not yet learn how to write the
test for the code written.
I would be happy to see any comments/suggestions.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Sep 11 2018, 8:59 AM
grimar edited the summary of this revision. (Show Details)Sep 11 2018, 9:00 AM
grimar updated this revision to Diff 164911.Sep 11 2018, 9:38 AM
  • Minor cosmetic cleanup.
clayborg requested changes to this revision.Sep 11 2018, 9:50 AM

Nice patch! A few minor fixes, see inlined comments.

source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
78 ↗(On Diff #164900)

Rename to GetHeaderByteSize() and do:

uint32_t DWARFCompileUnit::GetHeaderByteSize() {
  if (m_version < 5)
    return m_is_dwarf64 ? 23 : 11;
  switch (m_unit_type) {
    case llvm::dwarf::DW_UT_compile:
    case llvm::dwarf::DW_UT_partial:
      return 12;
    case llvm::dwarf::DW_UT_skeleton:
    case llvm::dwarf::DW_UT_split_compile:
      return 20;
    case llvm::dwarf::DW_UT_type:
    case llvm::dwarf::DW_UT_split_type:
      return 24;
  }
  llvm_unreachable("invalid UnitType.");
}
source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h
38–42 ↗(On Diff #164900)

Move this to .cpp file and inline contents of GetDWARF5HeaderSize() into the the body of the function.

45 ↗(On Diff #164900)

Remove this

source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
442 ↗(On Diff #164900)

"dirCount" is documented in the DWARF 5 spec to be a "ubyte" not a ULEB128

source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
557–558 ↗(On Diff #164900)

Revert signature and get info form m_cu. There are no other calls to AsCString() that manually specify a different symbol file and 64 bit value.

613–616 ↗(On Diff #164900)

Remove this as there are no other calls to AsCString() that manually specify a different symbol file and 64 bit value.

source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
77 ↗(On Diff #164900)

This doesn't seem to be needed in this patch? The SymbolFileDWARF and Is64Bit can be grabbed from the m_cu

This revision now requires changes to proceed.Sep 11 2018, 9:50 AM
grimar marked 5 inline comments as done.Sep 12 2018, 4:19 AM

Thanks for the review, Greg! I'll address your comments and add the test case when it will be ready. Hope soon.

grimar updated this revision to Diff 165112.Sep 12 2018, 10:17 AM
  • Addressed review comments.
  • Added test case.
  • Minor cosmetic changes.
grimar planned changes to this revision.Sep 12 2018, 10:20 AM

Will reupload.

clayborg requested changes to this revision.Sep 12 2018, 10:30 AM

Patch looks good. A few fixes mentioned in inlined comments. And I am unsure how the test suite will run with various compilers if they don't support the -gdwarf-5 flag. We might need to run obj2yaml on a binary and then yaml2obj it for the test to ensure we can run the test. We might be able to just symbolicate a few addresses in the test instead of requiring us to run something.

include/lldb/lldb-enumerations.h
675 ↗(On Diff #165112)

Add this after the eSectionTypeOther for backward compatibility.

source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
442 ↗(On Diff #165112)

We might verify that this is indeed correct by looking at compiler sources. Seems weird to limit dirs to 255

grimar added a comment.EditedSep 12 2018, 10:36 AM

Patch looks good. A few fixes mentioned in inlined comments. And I am unsure how the test suite will run with various compilers if they don't support the -gdwarf-5 flag. We might need to run obj2yaml on a binary and then yaml2obj it for the test to ensure we can run the test. We might be able to just symbolicate a few addresses in the test instead of requiring us to run something.

What about using the llvm-mc? I can convert c++ code to the assembler I think.
I'll try to investigate this problem tomorrow. Maybe we can just turn this test off somehow for that case.

Patch looks good. A few fixes mentioned in inlined comments. And I am unsure how the test suite will run with various compilers if they don't support the -gdwarf-5 flag. We might need to run obj2yaml on a binary and then yaml2obj it for the test to ensure we can run the test. We might be able to just symbolicate a few addresses in the test instead of requiring us to run something.

What about using the llvm-mc? I can convert c++ code to the assembler I think.
I'll try to investigate this problem tomorrow. Maybe we can just turn this test off somehow for that case.

Yeah, we want a test that goes along with this feature, but not sure how to test "does my compiler support this flag" in the test suite without reverting to llvm-mc or obj2yaml/yaml2obj

probinson added inline comments.
source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
442 ↗(On Diff #165112)

The directory count is a ULEB not a ubyte.

grimar added inline comments.Sep 13 2018, 4:17 AM
source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
442 ↗(On Diff #165112)

Oops. My mistake here, my initial version which used uleb128 was correct, but I looked at the wrong place too in the dwarf5 spec after that.

grimar updated this revision to Diff 165289.Sep 13 2018, 7:48 AM
grimar marked 2 inline comments as done.
  • Addressed comments.
  • Changed test case to use yaml2obj.
clayborg accepted this revision.Sep 13 2018, 8:37 AM

Thanks for doing all requested changes! Looks great.

source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp
442 ↗(On Diff #165112)

I incorrectly skimmed the DWARF 5 spec and looked at "directory_entry_format_count (ubyte)" and missed that this was for the directory entry format count... directories_count is indeed ULEB128.

This revision is now accepted and ready to land.Sep 13 2018, 8:37 AM
grimar retitled this revision from [LLDB] - Improve reporting source lines and variables (improved DWARF5 support). to [LLDB] - Improved DWARF5 support..Sep 13 2018, 10:06 AM
This revision was automatically updated to reflect the committed changes.