[Lldb-commits] [PATCH] D151919: [lldb][NFCI] Apply IndexEntry to DWARFUnitHeader outside of extraction
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdd76375c8009: [lldb][NFCI] Apply IndexEntry to
DWARFUnitHeader outside of extraction (authored by bulbazord).
Changed prior to commit:
https://reviews.llvm.org/D151919?vs=529769=557568#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151919/new/
https://reviews.llvm.org/D151919
Files:
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
@@ -76,6 +76,8 @@
}
uint32_t GetNextUnitOffset() const { return m_offset + m_length + 4; }
+ llvm::Error ApplyIndexEntry(const llvm::DWARFUnitIndex::Entry *index_entry);
+
static llvm::Expected
extract(const lldb_private::DWARFDataExtractor , DIERef::Section section,
lldb_private::DWARFContext _context,
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -877,11 +877,37 @@
return *m_func_aranges_up;
}
-llvm::Expected
-DWARFUnitHeader::extract(const DWARFDataExtractor ,
- DIERef::Section section,
- lldb_private::DWARFContext ,
- lldb::offset_t *offset_ptr) {
+llvm::Error DWARFUnitHeader::ApplyIndexEntry(
+const llvm::DWARFUnitIndex::Entry *index_entry) {
+ // We should only be calling this function when the index entry is not set and
+ // we have a valid one to set it to.
+ assert(index_entry);
+ assert(!m_index_entry);
+
+ if (m_abbr_offset)
+return llvm::createStringError(
+llvm::inconvertibleErrorCode(),
+"Package unit with a non-zero abbreviation offset");
+
+ auto *unit_contrib = index_entry->getContribution();
+ if (!unit_contrib || unit_contrib->getLength32() != m_length + 4)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Inconsistent DWARF package unit index");
+
+ auto *abbr_entry = index_entry->getContribution(llvm::DW_SECT_ABBREV);
+ if (!abbr_entry)
+return llvm::createStringError(
+llvm::inconvertibleErrorCode(),
+"DWARF package index missing abbreviation column");
+
+ m_abbr_offset = abbr_entry->getOffset();
+ m_index_entry = index_entry;
+ return llvm::Error::success();
+}
+
+llvm::Expected DWARFUnitHeader::extract(
+const DWARFDataExtractor , DIERef::Section section,
+lldb_private::DWARFContext , lldb::offset_t *offset_ptr) {
DWARFUnitHeader header;
header.m_offset = *offset_ptr;
header.m_length = data.GetDWARFInitialLength(offset_ptr);
@@ -905,42 +931,6 @@
header.m_type_offset = data.GetDWARFOffset(offset_ptr);
}
- if (context.isDwo()) {
-const llvm::DWARFUnitIndex *Index;
-if (header.IsTypeUnit()) {
- Index = ().getTUIndex();
- if (*Index)
-header.m_index_entry = Index->getFromHash(header.m_type_hash);
-} else {
- Index = ().getCUIndex();
- if (*Index && header.m_version >= 5 && header.m_dwo_id)
-header.m_index_entry = Index->getFromHash(*header.m_dwo_id);
-}
-if (!header.m_index_entry)
- header.m_index_entry = Index->getFromOffset(header.m_offset);
- }
-
- if (header.m_index_entry) {
-if (header.m_abbr_offset) {
- return llvm::createStringError(
- llvm::inconvertibleErrorCode(),
- "Package unit with a non-zero abbreviation offset");
-}
-auto *unit_contrib = header.m_index_entry->getContribution();
-if (!unit_contrib || unit_contrib->getLength32() != header.m_length + 4) {
- return llvm::createStringError(llvm::inconvertibleErrorCode(),
- "Inconsistent DWARF package unit index");
-}
-auto *abbr_entry =
-header.m_index_entry->getContribution(llvm::DW_SECT_ABBREV);
-if (!abbr_entry) {
- return llvm::createStringError(
- llvm::inconvertibleErrorCode(),
- "DWARF package index missing abbreviation column");
-}
-header.m_abbr_offset = abbr_entry->getOffset();
- }
-
bool length_OK = data.ValidOffset(header.GetNextUnitOffset() - 1);
bool version_OK = SymbolFileDWARF::SupportedVersion(header.m_version);
bool addr_size_OK = (header.m_addr_size == 2) || (header.m_addr_size == 4) ||
@@ -970,11 +960,30 @@
DIERef::Section section, lldb::offset_t *offset_ptr) {
assert(debug_info.ValidOffset(*offset_ptr));
- auto expected_header = DWARFUnitHeader::extract(
- debug_info, section, dwarf.GetDWARFContext(), offset_ptr);
+ DWARFContext = dwarf.GetDWARFContext();
+ auto expected_header =
+
[Lldb-commits] [PATCH] D151919: [lldb][NFCI] Apply IndexEntry to DWARFUnitHeader outside of extraction
bulbazord updated this revision to Diff 529769.
bulbazord added a comment.
Changed a bit of formatting to follow LLVM style more closely.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151919/new/
https://reviews.llvm.org/D151919
Files:
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
@@ -75,6 +75,8 @@
}
uint32_t GetNextUnitOffset() const { return m_offset + m_length + 4; }
+ llvm::Error ApplyIndexEntry(const llvm::DWARFUnitIndex::Entry *index_entry);
+
static llvm::Expected
extract(const lldb_private::DWARFDataExtractor , DIERef::Section section,
lldb_private::DWARFContext _context,
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -875,11 +875,37 @@
return *m_func_aranges_up;
}
-llvm::Expected
-DWARFUnitHeader::extract(const DWARFDataExtractor ,
- DIERef::Section section,
- lldb_private::DWARFContext ,
- lldb::offset_t *offset_ptr) {
+llvm::Error DWARFUnitHeader::ApplyIndexEntry(
+const llvm::DWARFUnitIndex::Entry *index_entry) {
+ // We should only be calling this function when the index entry is not set and
+ // we have a valid one to set it to.
+ assert(index_entry);
+ assert(!m_index_entry);
+
+ if (m_abbr_offset)
+return llvm::createStringError(
+llvm::inconvertibleErrorCode(),
+"Package unit with a non-zero abbreviation offset");
+
+ auto *unit_contrib = index_entry->getContribution();
+ if (!unit_contrib || unit_contrib->getLength32() != m_length + 4)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Inconsistent DWARF package unit index");
+
+ auto *abbr_entry = index_entry->getContribution(llvm::DW_SECT_ABBREV);
+ if (!abbr_entry)
+return llvm::createStringError(
+llvm::inconvertibleErrorCode(),
+"DWARF package index missing abbreviation column");
+
+ m_abbr_offset = abbr_entry->getOffset();
+ m_index_entry = index_entry;
+ return llvm::Error::success();
+}
+
+llvm::Expected DWARFUnitHeader::extract(
+const DWARFDataExtractor , DIERef::Section section,
+lldb_private::DWARFContext , lldb::offset_t *offset_ptr) {
DWARFUnitHeader header;
header.m_offset = *offset_ptr;
header.m_length = data.GetDWARFInitialLength(offset_ptr);
@@ -903,42 +929,6 @@
header.m_type_offset = data.GetDWARFOffset(offset_ptr);
}
- if (context.isDwo()) {
-const llvm::DWARFUnitIndex *Index;
-if (header.IsTypeUnit()) {
- Index = ().getTUIndex();
- if (*Index)
-header.m_index_entry = Index->getFromHash(header.m_type_hash);
-} else {
- Index = ().getCUIndex();
- if (*Index && header.m_version >= 5 && header.m_dwo_id)
-header.m_index_entry = Index->getFromHash(*header.m_dwo_id);
-}
-if (!header.m_index_entry)
- header.m_index_entry = Index->getFromOffset(header.m_offset);
- }
-
- if (header.m_index_entry) {
-if (header.m_abbr_offset) {
- return llvm::createStringError(
- llvm::inconvertibleErrorCode(),
- "Package unit with a non-zero abbreviation offset");
-}
-auto *unit_contrib = header.m_index_entry->getContribution();
-if (!unit_contrib || unit_contrib->getLength32() != header.m_length + 4) {
- return llvm::createStringError(llvm::inconvertibleErrorCode(),
- "Inconsistent DWARF package unit index");
-}
-auto *abbr_entry =
-header.m_index_entry->getContribution(llvm::DW_SECT_ABBREV);
-if (!abbr_entry) {
- return llvm::createStringError(
- llvm::inconvertibleErrorCode(),
- "DWARF package index missing abbreviation column");
-}
-header.m_abbr_offset = abbr_entry->getOffset();
- }
-
bool length_OK = data.ValidOffset(header.GetNextUnitOffset() - 1);
bool version_OK = SymbolFileDWARF::SupportedVersion(header.m_version);
bool addr_size_OK = (header.m_addr_size == 2) || (header.m_addr_size == 4) ||
@@ -968,11 +958,30 @@
DIERef::Section section, lldb::offset_t *offset_ptr) {
assert(debug_info.ValidOffset(*offset_ptr));
- auto expected_header = DWARFUnitHeader::extract(
- debug_info, section, dwarf.GetDWARFContext(), offset_ptr);
+ DWARFContext = dwarf.GetDWARFContext();
+ auto expected_header =
+ DWARFUnitHeader::extract(debug_info, section, context, offset_ptr);
if (!expected_header)
return expected_header.takeError();
+ if
[Lldb-commits] [PATCH] D151919: [lldb][NFCI] Apply IndexEntry to DWARFUnitHeader outside of extraction
bulbazord added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:882 + // we have a valid one to set it to. + assert(index_entry); + assert(!m_index_entry); fdeazeve wrote: > If LLVM's function is like this as well, we shouldn't do anything now and > just refactor it later once we make the switch. But a pointer parameter in a > function that asserts not null immediately is more expressive as a reference. > Note that, in the code where this is called, we have a: > > ``` > if (ptr) > ApplyIndex(ptr) > ``` > > So the assert is not needed, as the callee respects the contract and can do a > `*ptr` > Yes, this was my thought process as well. I wrote it this way because LLVM's function is just like this. I'm also touching that equivalent function in LLVM in a different way (adding error handling, see https://reviews.llvm.org/D151933). Let's rework this in a follow-up? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151919/new/ https://reviews.llvm.org/D151919 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D151919: [lldb][NFCI] Apply IndexEntry to DWARFUnitHeader outside of extraction
fdeazeve accepted this revision. fdeazeve added a comment. I'm not exactly familiar with DWOs, but the code motions LGTM! Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:882 + // we have a valid one to set it to. + assert(index_entry); + assert(!m_index_entry); If LLVM's function is like this as well, we shouldn't do anything now and just refactor it later once we make the switch. But a pointer parameter in a function that asserts not null immediately is more expressive as a reference. Note that, in the code where this is called, we have a: ``` if (ptr) ApplyIndex(ptr) ``` So the assert is not needed, as the callee respects the contract and can do a `*ptr` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151919/new/ https://reviews.llvm.org/D151919 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D151919: [lldb][NFCI] Apply IndexEntry to DWARFUnitHeader outside of extraction
bulbazord added inline comments.
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:889
+"Package unit with a non-zero abbreviation offset");
+
+ auto *unit_contrib = index_entry->getContribution();
aprantl wrote:
> I know this was also in the original code, so you may not know either: Should
> these error messages be limited to dwarf packages (.dwp) or should they be
> more generic?
I'm not sure, but my understanding is that we only apply the index entry to the
header in DWO contexts, so I assume DWP is involved.
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:980
+ entry = index.getFromOffset(expected_header->GetOffset());
+if (entry) {
+ if (llvm::Error err = expected_header->ApplyIndexEntry(entry)) {
aprantl wrote:
> llvm coding style would delete most of these {} ?
Sounds good, I'll update.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151919/new/
https://reviews.llvm.org/D151919
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D151919: [lldb][NFCI] Apply IndexEntry to DWARFUnitHeader outside of extraction
aprantl accepted this revision.
aprantl added inline comments.
This revision is now accepted and ready to land.
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:889
+"Package unit with a non-zero abbreviation offset");
+
+ auto *unit_contrib = index_entry->getContribution();
I know this was also in the original code, so you may not know either: Should
these error messages be limited to dwarf packages (.dwp) or should they be more
generic?
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:980
+ entry = index.getFromOffset(expected_header->GetOffset());
+if (entry) {
+ if (llvm::Error err = expected_header->ApplyIndexEntry(entry)) {
llvm coding style would delete most of these {} ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151919/new/
https://reviews.llvm.org/D151919
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D151919: [lldb][NFCI] Apply IndexEntry to DWARFUnitHeader outside of extraction
bulbazord created this revision.
bulbazord added reviewers: aprantl, fdeazeve, rastogishubham, JDevlieghere.
Herald added a subscriber: arphaman.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
I plan on replacing LLDB's DWARFUnitHeader implementation with LLVM's.
LLVM's DWARFUnitHeader::extract applies the DWARFUnitIndex::Entry to a
given DWARFUnitHeader outside of the extraction because the index entry
is only relevant to one place where we may parse DWARFUnitHeaders
(specifically when we're creating a DWARFUnit in a DWO context). To ease
the transition, I've reshaped LLDB's implementation to look closer to
LLVM's.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D151919
Files:
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.h
@@ -75,6 +75,8 @@
}
uint32_t GetNextUnitOffset() const { return m_offset + m_length + 4; }
+ llvm::Error ApplyIndexEntry(const llvm::DWARFUnitIndex::Entry *index_entry);
+
static llvm::Expected
extract(const lldb_private::DWARFDataExtractor , DIERef::Section section,
lldb_private::DWARFContext _context,
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -875,11 +875,37 @@
return *m_func_aranges_up;
}
-llvm::Expected
-DWARFUnitHeader::extract(const DWARFDataExtractor ,
- DIERef::Section section,
- lldb_private::DWARFContext ,
- lldb::offset_t *offset_ptr) {
+llvm::Error DWARFUnitHeader::ApplyIndexEntry(
+const llvm::DWARFUnitIndex::Entry *index_entry) {
+ // We should only be calling this function when the index entry is not set and
+ // we have a valid one to set it to.
+ assert(index_entry);
+ assert(!m_index_entry);
+
+ if (m_abbr_offset)
+return llvm::createStringError(
+llvm::inconvertibleErrorCode(),
+"Package unit with a non-zero abbreviation offset");
+
+ auto *unit_contrib = index_entry->getContribution();
+ if (!unit_contrib || unit_contrib->getLength32() != m_length + 4)
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Inconsistent DWARF package unit index");
+
+ auto *abbr_entry = index_entry->getContribution(llvm::DW_SECT_ABBREV);
+ if (!abbr_entry)
+return llvm::createStringError(
+llvm::inconvertibleErrorCode(),
+"DWARF package index missing abbreviation column");
+
+ m_abbr_offset = abbr_entry->getOffset();
+ m_index_entry = index_entry;
+ return llvm::Error::success();
+}
+
+llvm::Expected DWARFUnitHeader::extract(
+const DWARFDataExtractor , DIERef::Section section,
+lldb_private::DWARFContext , lldb::offset_t *offset_ptr) {
DWARFUnitHeader header;
header.m_offset = *offset_ptr;
header.m_length = data.GetDWARFInitialLength(offset_ptr);
@@ -903,42 +929,6 @@
header.m_type_offset = data.GetDWARFOffset(offset_ptr);
}
- if (context.isDwo()) {
-const llvm::DWARFUnitIndex *Index;
-if (header.IsTypeUnit()) {
- Index = ().getTUIndex();
- if (*Index)
-header.m_index_entry = Index->getFromHash(header.m_type_hash);
-} else {
- Index = ().getCUIndex();
- if (*Index && header.m_version >= 5 && header.m_dwo_id)
-header.m_index_entry = Index->getFromHash(*header.m_dwo_id);
-}
-if (!header.m_index_entry)
- header.m_index_entry = Index->getFromOffset(header.m_offset);
- }
-
- if (header.m_index_entry) {
-if (header.m_abbr_offset) {
- return llvm::createStringError(
- llvm::inconvertibleErrorCode(),
- "Package unit with a non-zero abbreviation offset");
-}
-auto *unit_contrib = header.m_index_entry->getContribution();
-if (!unit_contrib || unit_contrib->getLength32() != header.m_length + 4) {
- return llvm::createStringError(llvm::inconvertibleErrorCode(),
- "Inconsistent DWARF package unit index");
-}
-auto *abbr_entry =
-header.m_index_entry->getContribution(llvm::DW_SECT_ABBREV);
-if (!abbr_entry) {
- return llvm::createStringError(
- llvm::inconvertibleErrorCode(),
- "DWARF package index missing abbreviation column");
-}
-header.m_abbr_offset = abbr_entry->getOffset();
- }
-
bool length_OK = data.ValidOffset(header.GetNextUnitOffset() - 1);
bool version_OK = SymbolFileDWARF::SupportedVersion(header.m_version);
bool addr_size_OK = (header.m_addr_size == 2) ||
