This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)
ClosedPublic

Authored by jankratochvil on Jul 21 2021, 10:47 AM.

Details

Summary

Fix D98289 so that it works even for 2nd..nth compilation unit (.debug_rnglists).

Diff Detail

Event Timeline

jankratochvil created this revision.Jul 21 2021, 10:47 AM
jankratochvil requested review of this revision.Jul 21 2021, 10:47 AM

As far as I understand it, you need a specially constructed llvm::DWARFDebugRnglistTable object so that DWARFUnit::FindRnglistFromOffset() can call its findList() method and get a list of records located at a specific offset. It looks like a newly created llvm::DWARFDebugRnglistTable object has its Header.Length set to 0, so it already works as required for the usage, and there is no need to fill its fields with artificial values that do not represent real content of the section. Am I right?

llvm/lib/DebugInfo/DWARF/DWARFListTable.cpp
54 ↗(On Diff #360518)

The line looks suspicious because Data is a local variable that is destroyed right after the statement.

jankratochvil retitled this revision from [llvm+lldb] 2/2: Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC) to [llvm+lldb] 4/4: Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC).
jankratochvil edited the summary of this revision. (Show Details)
jankratochvil retitled this revision from [llvm+lldb] 4/4: Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC) to 3/3: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC).Aug 4 2021, 10:39 AM
jankratochvil marked an inline comment as done.EditedAug 4 2021, 11:03 AM

As far as I understand it, you need a specially constructed llvm::DWARFDebugRnglistTable object so that DWARFUnit::FindRnglistFromOffset() can call its findList() method and get a list of records located at a specific offset.

Yes.

It looks like a newly created llvm::DWARFDebugRnglistTable object has its Header.Length set to 0,

But that is wrong Length, the correct one is to cover the whole section which I set by:

HeaderData.Length = Data.size() - dwarf::getUnitLengthFieldByteSize(Format);

so it already works as required for the usage, and there is no need to fill its fields with artificial values that do not represent real content of the section. Am I right?

One needs to set at least AddrSize and OffsetEntryCount as callers do use it. Not sure now with the other fields but in such case one should remove them also from existing DWARFListTableHeader::extract. DWARFListTableHeader::create should be as much similar to DWARFListTableHeader::extract as possible. Maybe we could make some [nfc] patches to make DWARFListTableHeader::Header smaller but it is now only 16 bytes already so I do not find that much useful. It would be also orthogonal to this patch.

llvm/lib/DebugInfo/DWARF/DWARFListTable.cpp
54 ↗(On Diff #360518)

You are right. Although existing DWARFListTableHeader::extract had the same problem from where I did copy-paste it. Filed for it D107470.

jankratochvil marked an inline comment as done.

As far as I understand it, you need a specially constructed llvm::DWARFDebugRnglistTable object so that DWARFUnit::FindRnglistFromOffset() can call its findList() method and get a list of records located at a specific offset.

Yes.

It looks like a newly created llvm::DWARFDebugRnglistTable object has its Header.Length set to 0,

But that is wrong Length, the correct one is to cover the whole section which I set by:

HeaderData.Length = Data.size() - dwarf::getUnitLengthFieldByteSize(Format);

This value is not incorrect, but a special one, with custom handling. DWARFUnit::findRnglistFromOffset(uint64_t Offset) fetches the list without creating a fake header.

so it already works as required for the usage, and there is no need to fill its fields with artificial values that do not represent real content of the section. Am I right?

One needs to set at least AddrSize and OffsetEntryCount as callers do use it.

Could you please point me to that usage? I am not that familiar with the LLDB code.

jankratochvil retitled this revision from 3/3: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC) to [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC).

One needs to set at least AddrSize and OffsetEntryCount as callers do use it.

Could you please point me to that usage? I am not that familiar with the LLDB code.

Those are used by DWARFUnit::GetRnglistOffset->llvm::DWARFDebugRnglistTable::getOffsetEntry. But in such case llvm::DWARFDebugRnglistTable::extractHeaderAndOffsets were already called that time.
So that is not the code path I need for DWARF not using DW_AT_rnglists_base.

I understand now the Header.length==0 case so the fix is simple now, thanks for showing it to me.

The code looks good, but please improve the comments and wait for approval from a more LLDB-knowledgeable person than me,

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
441

Please extend the comment to emphasize that even if DW_AT_rnglists_base is missing and DW_FORM_rnglistx cannot be handled, returning a default-constructed Table allows DW_FORM_sec_offset to be supported.

1007

This comment is quite misleading as it references offset which is the argument of the method.

jankratochvil marked 2 inline comments as done.
jankratochvil edited the summary of this revision. (Show Details)

@dblaikie do you think it is OK for check-in now?

I assume there's already test coverage for rnglistx in debug_info.dwo/split unit? (because in that case there's no rnglists_base, but rnglistx is usable) - could you explain how this code avoids treating the split unit rnglists_base == 0 case as "there is no rnglists_base and so rnglistx isn't usable"?

I assume there's already test coverage for rnglistx in debug_info.dwo/split unit? (because in that case there's no rnglists_base, but rnglistx is usable)

I admit I did not check it, thanks for catching it. But it is already tested by lldb/test/Shell/SymbolFile/DWARF/x86/debug_rnglists-dwo.s implemented by Pavel Labath in 2019. It is using DWO DW_TAG_lexical_block->DW_AT_ranges->DW_FORM_rnglistx.

  • could you explain how this code avoids treating the split unit rnglists_base == 0 case as "there is no rnglists_base and so rnglistx isn't usable"?

m_ranges_base is not zero in such case as it has been set from the skeleton.

dblaikie accepted this revision.Aug 17 2021, 12:48 PM

I assume there's already test coverage for rnglistx in debug_info.dwo/split unit? (because in that case there's no rnglists_base, but rnglistx is usable)

I admit I did not check it, thanks for catching it. But it is already tested by lldb/test/Shell/SymbolFile/DWARF/x86/debug_rnglists-dwo.s implemented by Pavel Labath in 2019. It is using DWO DW_TAG_lexical_block->DW_AT_ranges->DW_FORM_rnglistx.

  • could you explain how this code avoids treating the split unit rnglists_base == 0 case as "there is no rnglists_base and so rnglistx isn't usable"?

m_ranges_base is not zero in such case as it has been set from the skeleton.

ah, OK - was going to say that setting it from the skeleton would be incorrect (for DWARF 5, at least) - but I see that's handled a few lines down: https://github.com/llvm/llvm-project/blob/f58a642da19c64cb6ee1badb0f176a872c1d7a0a/lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp#L110

Looks all good to me, then.

This revision is now accepted and ready to land.Aug 17 2021, 12:48 PM
This revision was landed with ongoing or failed builds.Aug 17 2021, 1:19 PM
This revision was automatically updated to reflect the committed changes.
jankratochvil retitled this revision from [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC) to [lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC).Aug 17 2021, 1:21 PM