This is an archive of the discontinued LLVM Phabricator instance.

[llvm+lldb] Remove dead-code in DWARFListTableHeader::extract modifying DWARFDataExtractor
Needs ReviewPublic

Authored by jankratochvil on Aug 4 2021, 8:33 AM.

Details

Summary

@ikudrin has correctly noticed modifying a copy of DWARFDataExtractor makes no sense. Removed it as it currently has no effect. In fact one caller needs it as otherwise it assumes .debug_rnglists width from ELF width which in practice works but it is not correct. All callers of DWARFListTableHeader::extract:

Callers of GetRnglistTable:

Diff Detail

Event Timeline

jankratochvil created this revision.Aug 4 2021, 8:33 AM
jankratochvil retitled this revision from 2/4: [llvm+lldb] Remove dead-code in DWARFListTableHeader::extract modifying DWARFDataExtractor to 2/3: [llvm+lldb] Remove dead-code in DWARFListTableHeader::extract modifying DWARFDataExtractor.Aug 4 2021, 10:39 AM
ikudrin added inline comments.Aug 11 2021, 8:22 AM
llvm/unittests/DebugInfo/DWARF/DWARFListTableTest.cpp
128

This looks odd. DWARFListTableBase::findList() should not require the setting to be done in the calling code if it can apply it itself.

jankratochvil planned changes to this revision.Aug 11 2021, 8:25 AM
jankratochvil marked an inline comment as done.Aug 12 2021, 9:22 AM
jankratochvil added inline comments.
llvm/unittests/DebugInfo/DWARF/DWARFListTableTest.cpp
128

I agree but as I have found it is not so simple. findList is being used also with uninitialized headers: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp DWARFUnit::findRnglistFromOffset
So I had to implement DWARFListTableBase::setAddrSize which is a bit ugly. One cannot find appropriate .debug_loclists header just for the requested offset without DW_AT_rnglists_base which may be missing, that is what D106466 is about. Or one could use DWARFListTableHeader::create from that patch but that looks to me as too much complicated.

In fact the codebase is already designed so that caller has to prepare proper DWARFDataExtractor - llvm::DWARFDebugLoclists and llvm::DWARFDebugLoc also rely on it, I tried to implement this callee approach for them as: https://www.jankratochvil.net/t/gccoffset1-2-2b.patch
But I am not going to submit it as one either has to provide address size to both DWARFDataExtractor and llvm::DWARFDebugLoc* constructors or otherwise DWARFDataExtractor would have unset address size (implemented in the patch) which is also a bit ugly.

So personally I would choose the previous patch but I am fine also with this patch.

ikudrin added inline comments.Aug 12 2021, 11:49 PM
llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h
285–289

if Header.Length is zero that means that there is no header discovered for the table. That can be handled accordingly and no need to fill header fields with artificial values based on a particular use.

jankratochvil marked an inline comment as done.
jankratochvil retitled this revision from 2/3: [llvm+lldb] Remove dead-code in DWARFListTableHeader::extract modifying DWARFDataExtractor to [llvm+lldb] Remove dead-code in DWARFListTableHeader::extract modifying DWARFDataExtractor.
jankratochvil marked an inline comment as done.Aug 14 2021, 1:10 PM
jankratochvil added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h
285–289

I agree, I still did not get the length==0 case, thanks for showing it.
(It would be nice to have a more simple class for that case.)

DWARFDebugInfo.TestRnglistsAddressSize, DWARFListTableHeader.AddressSize64Offset, and DWARFListTableHeader.AddressSize32Offset pass without applying the patch. Why adding them?

llvm/unittests/DebugInfo/DWARF/DWARFListTableTest.cpp
123

The rest of the test makes no sense if extractHeaderAndOffsets() returns an error.

129–130

If these checks fail, the execution of the test should be terminated.

jankratochvil marked 3 inline comments as done.Aug 16 2021, 11:24 AM

DWARFDebugInfo.TestRnglistsAddressSize, DWARFListTableHeader.AddressSize64Offset, and DWARFListTableHeader.AddressSize32Offset pass without applying the patch. Why adding them?

This code has shown it is prone to errors so future changes could break some other combination. Better code coverage by the testsuite is AFAIK always preferred.