Page MenuHomePhabricator

Remove Support for DWARF64
ClosedPublic

Authored by zturner on Mar 11 2019, 3:58 PM.

Details

Summary

LLVM doesn't produce DWARF64, and neither does GCC. LLDB's support for DWARF64 is only partial, and if enabled appears to also not work. It also appears to have no test coverage. Removing this makes merging LLVM and LLDB's DWARF parsing implementations simpler.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

zturner created this revision.Mar 11 2019, 3:58 PM
aprantl accepted this revision.Mar 11 2019, 4:05 PM

LGTM, perhaps let's give this a day for others to chime in in case they missed the thread on lldb-dev.

This revision is now accepted and ready to land.Mar 11 2019, 4:05 PM

What part of parsing DWARF64 wasn't working? I think the SPARC compiler was the only one producing DWARF64. Be a shame to lose the support. What is the plan here after this patch?

What part of parsing DWARF64 wasn't working? I think the SPARC compiler was the only one producing DWARF64.

This patch originated from a thread on lldb-dev where I asked if anyone knows the status. The first response was from @jankratochvil at Red Hat who gave this example:

IMO there isn't as for example:
lldb/source/Plugins/SymbolFile/DWARF/DIERef.cpp
is using bits 32..63 for additional info (DWO file offset/index for example)
while only bits 0..31 are used for DIE offset inside .debug_info section.

However, presumably this was identified from Jan simply looking at the code. There could be other such examples.

Be a shame to lose the support. What is the plan here after this patch?

Currently I'm doing some very preliminary investigation into merging LLVM and LLDB's DWARF parsers. Initially, I started by updating LLVM's DataExtractor class to accept size_t offsets instead of uint32_t, however that quickly grew into a very large patch.

To answer your question: I think the plan would be to first standardize on a single DWARF parser, and then if / when we decide to support DWARF64, do it in one place (and importantly, make sure it has some corresponding test coverage).

My main concern with the LLVM DWARF parser is all of the asserts in the code. If you attempt to use a DWARFDIE without first checking it for validity, it will crash on you instead of returning a good error or default value. That makes me really nervous as we shouldn't just crash the debugger. The switching over won't be too hard, just the fallout from the LLDB versions of the class that do error checking and return good error/default values and LLVM being very strict.

My main concern with the LLVM DWARF parser is all of the asserts in the code. If you attempt to use a DWARFDIE without first checking it for validity, it will crash on you instead of returning a good error or default value. That makes me really nervous as we shouldn't just crash the debugger. The switching over won't be too hard, just the fallout from the LLDB versions of the class that do error checking and return good error/default values and LLVM being very strict.

Sure, I'm prepared to deal all that appropriately. I don't plan to regress LLDB's stability in the process.

That's why for now I'm just doing very small preliminary steps to get the two interfaces to be closer to each other and simplify the problem space. We can worry about the asserts and all of that when we actually start moving pieces of LLDB to use LLVM's classes (which isn't in this patch).

My main concern with the LLVM DWARF parser is all of the asserts in the code. If you attempt to use a DWARFDIE without first checking it for validity, it will crash on you instead of returning a good error or default value. That makes me really nervous as we shouldn't just crash the debugger. The switching over won't be too hard, just the fallout from the LLDB versions of the class that do error checking and return good error/default values and LLVM being very strict.

Sure, I'm prepared to deal all that appropriately. I don't plan to regress LLDB's stability in the process.

Sounds good.

That's why for now I'm just doing very small preliminary steps to get the two interfaces to be closer to each other and simplify the problem space. We can worry about the asserts and all of that when we actually start moving pieces of LLDB to use LLVM's classes (which isn't in this patch).

I look forward to seeing this transition. I was planning on working on that right before I left Apple, but I left and didn't have the time after starting a new job. I will be happy to help review any changes as I did a lot of cleanup in the LLVM DWARF parser in preparation for the transition, so we should be in good shape.

My main concern with the LLVM DWARF parser is all of the asserts in the code. If you attempt to use a DWARFDIE without first checking it for validity, it will crash on you instead of returning a good error or default value. That makes me really nervous as we shouldn't just crash the debugger. The switching over won't be too hard, just the fallout from the LLDB versions of the class that do error checking and return good error/default values and LLVM being very strict.

Sure, I'm prepared to deal all that appropriately. I don't plan to regress LLDB's stability in the process.

That's why for now I'm just doing very small preliminary steps to get the two interfaces to be closer to each other and simplify the problem space. We can worry about the asserts and all of that when we actually start moving pieces of LLDB to use LLVM's classes (which isn't in this patch).

A long term plan of moving LLVM's parser away from asserts and toward error reporting on bad input would also make the binutils that try to read DWARF more robust and useful for trying to diagnose bad object files. I'm all for it.

jankratochvil added inline comments.Mar 12 2019, 8:10 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
263 ↗(On Diff #190173)

Here should be an error that DWARF64 is not supported.

lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
170 ↗(On Diff #190173)

Delete the assert?

327 ↗(On Diff #190173)

Delete the assert?

zturner marked an inline comment as done.Mar 12 2019, 10:11 AM

It seems like we're mostly in agreement on the direction of the patch, so if there's no outstanding comments I'll submit at the end of the day.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
263 ↗(On Diff #190173)

How do we report that error? This function doesn't return an error code, and neither does the function that calls it (I didn't look higher than that), and we don't build with exceptions. We can assert, but we try to avoid asserts (although admittedly, we're going to crash anyway). I can try to do more work to propagate errors up the stack, but at this point this whole file (and even most of the folder that the file is in) is in theory going to be going away soon as we converge on LLVM's DWARF parser, so it may make sense to just deal with the problem at that level.

zturner marked an inline comment as done.Mar 12 2019, 10:12 AM
zturner added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
170 ↗(On Diff #190173)

Good catch. I'll do this (and the other one) before submitting.

aprantl added inline comments.Mar 12 2019, 10:13 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
89 ↗(On Diff #190173)

Not your code, but this application of llvm_unreachable seems suspicious unless we can guarantee that we already checked for a supported format before entering here.

jankratochvil added inline comments.Mar 12 2019, 10:31 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.cpp
263 ↗(On Diff #190173)

Just that there should be something, at least a FIXME comment.
There is dwarf2Data->GetObjectFile()->GetModule()->ReportError() but it will currently lock up since implementation of multithreaded DWARF reading. But there are AFAIK already many such errors which do not happen in realworld but they would lock up if they did. I agree returning an error value is the proper solution, I have no idea when it is the right time to implement that.

My main concern with the LLVM DWARF parser is all of the asserts in the code. If you attempt to use a DWARFDIE without first checking it for validity, it will crash on you instead of returning a good error or default value. That makes me really nervous as we shouldn't just crash the debugger. The switching over won't be too hard, just the fallout from the LLDB versions of the class that do error checking and return good error/default values and LLVM being very strict.

Sure, I'm prepared to deal all that appropriately. I don't plan to regress LLDB's stability in the process.

That's why for now I'm just doing very small preliminary steps to get the two interfaces to be closer to each other and simplify the problem space. We can worry about the asserts and all of that when we actually start moving pieces of LLDB to use LLVM's classes (which isn't in this patch).

A long term plan of moving LLVM's parser away from asserts and toward error reporting on bad input would also make the binutils that try to read DWARF more robust and useful for trying to diagnose bad object files. I'm all for it.

Agreed, and we've been doing this for new patches for a while now. However, I very strongly prefer having asserts over "returning a default value", which only hides real bugs.

Agreed, and we've been doing this for new patches for a while now. However, I very strongly prefer having asserts over "returning a default value", which only hides real bugs.

I think everyone is on the same page here, but it doesn't hurt to explicitly repeat this :-)

  • Assertions should be used liberally to assert internal consistency and to enforce contracts in the API. Basically when you write an assertion, you should be already convinced that it will always hold.
  • Assertions may never be used to handle invalid external input in a parser. Invalid external input must use error handling, expected, optional, ...
Closed by commit rLLDB355975: Remove support for DWARF64. (authored by zturner, committed by ). · Explain WhyMar 12 2019, 1:51 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2019, 1:51 PM