[PATCH] D45170: Cleanup DWARFCompileUnit and DWARFUnit in preparation for adding DWARFTypeUnit

[Adrian Prantl via llvm-commits]

> On Apr 6, 2018, at 1:52 PM, Greg Clayton <clayborg at gmail.com> wrote:
> 
> 
> 
>> On Apr 6, 2018, at 10:32 AM, Jan Kratochvil <jan.kratochvil at redhat.com> wrote:
>> 
>> On Fri, 06 Apr 2018 18:58:27 +0200, Davide Italiano wrote:
>>> Yes, I definitely agree. It's clear this needs more discussion, so I
>>> don't think it's reasonable if we revert this for now and
>>> reconsider.
>> 
>> I have reverted this https://reviews.llvm.org/D45170 by:
>>       https://reviews.llvm.org/rL329423
>> 
>> 
>>> I'll also take a look at the interfaces in LLVM to get a better sense
>>> of what should be done. If nobody does this, I'll probably get to the
>>> revert by the end of the day.
>> 
>> I admit my DWARFUnit was in part alignment with LLVM's DWARFUnit (so that
>> there is one abstract superclass) but at the same time even more
>> differentiating it (as LLVM has no DWARFPartialUnit and DWARFTypeUnit will
>> need another intermediate superclass which I call DWARFSomeNameUnit in that
>> commit text above).
>> 
>> I was sure glad with my DWARFUnit https://reviews.llvm.org/D40466 approval to
>> be able to commit my DWZ support on top of it making LLDB usable on Red Hat
>> systems. But I understand LLDB code unification with LLVM is more important.
>> The DWZ patchset now waits on https://reviews.llvm.org/D40467 approval.
>> Greg did not like it much so I am now working on new DWARFOneFileOffset being
>> type-incompatible to dw_offset_t so that both types can be used safely without
>> accidentally interchanging each other; instead of my current relying on unsafe
>> type-compatible "dw_offset_t offset" vs. "dw_offset_t file_offset" parameters
>> naming.
>> 
>> Maybe LLDB should really reuse LLVM DWARFUnit first and I will implement DWZ
>> already on top of LLVM's DWARFUnit? I have no idea myself now how complex task
>> is the reuse of LLVM DWARFUnit. For the LLVM DWARFUnit reusal this Greg's
>> commit would be sure fine; just completely unrelated to why+how I made this
>> DWARFUnit split myself.
> 
> I have run into issues trying to use the LLVM ecosystem for object file parsing and for DWARF parsing. If anything in the object file is not exactly right LLVM will return an error. A few things I have run into is some ELF files produced by some tools don't correctly align their section headers and LLVM refuses to open the file. Switching to the LLVM DWARF parser will make us crash more as the DWARF classes there have many rules on using them. DWARFDIE will crash if you try to ask it to do anything and haven't checked if the object is valid. That is different from our classes where we just return an error or invalid value if you use and empty DWARFDIE. So we are close, but our code won't assert and kill the program. Switching over to the LLVM parser will require some detailed work and will take some time.
> 
> That being said, I am confused as to why this was reverted. The code I added mirrors the LLVM code a bit better, and yes it will require some reworking of your patches.

I agree, I don't think reverting was necessary / the right action in this case. We should move to make the interface of the LLDB DWARF parser more like the LLVM one. If the LLVM interface is not good for our needs we should evolve both at the same time. Since there is a very large overlap between LLDB developers and LLVM libDebugInfo developers this should not cause any problems. At this point we should avoid any changes that bring the two further apart.

-- adrian

> The DWARFUnit having an accessor to give out a DWARFCompileUnit was really confusing and not the right layering. So I fixed the layering. I need to submit .debug_types patches and that patch was needed for this and now I am back to square one.