[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
jankratochvil added inline comments.
Comment at: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:179
+ if (!m_die_array.empty()) {
+lldbassert(!m_first_die || m_first_die == m_die_array.front());
+m_first_die = m_die_array.front();
labath wrote:
> xbolva00 wrote:
> > xbolva00 wrote:
> > > @jankratochvil is this correct assert? Our downstream lldb based on lldb
> > > 7 with our custom targets hits this assert. If we remove it, we see no
> > > obvious breakages.
> > cc @labath
> I agree with Jan. The two dies should be the same. The fact that they aren't
> probably means there is a bug somewhere. It would be good to know how the two
> dies differ and what is the input dwarf they are generated from.
A fix of this assertion is now submitted as: D53255
Repository:
rL LLVM
https://reviews.llvm.org/D46810
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
labath added inline comments.
Herald added a subscriber: arphaman.
Comment at: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:179
+ if (!m_die_array.empty()) {
+lldbassert(!m_first_die || m_first_die == m_die_array.front());
+m_first_die = m_die_array.front();
xbolva00 wrote:
> xbolva00 wrote:
> > @jankratochvil is this correct assert? Our downstream lldb based on lldb 7
> > with our custom targets hits this assert. If we remove it, we see no
> > obvious breakages.
> cc @labath
I agree with Jan. The two dies should be the same. The fact that they aren't
probably means there is a bug somewhere. It would be good to know how the two
dies differ and what is the input dwarf they are generated from.
Repository:
rL LLVM
https://reviews.llvm.org/D46810
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
jankratochvil added a comment. @xbolva00 As you gave no reprodudcer it would be good to know how those two `DWARFDebugInfoEntry`s differ. Without that I can look at it more only next week - travelling. Repository: rL LLVM https://reviews.llvm.org/D46810 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
xbolva00 added inline comments.
Comment at: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:179
+ if (!m_die_array.empty()) {
+lldbassert(!m_first_die || m_first_die == m_die_array.front());
+m_first_die = m_die_array.front();
xbolva00 wrote:
> @jankratochvil is this correct assert? Our downstream lldb based on lldb 7
> with our custom targets hits this assert. If we remove it, we see no obvious
> breakages.
cc @labath
Repository:
rL LLVM
https://reviews.llvm.org/D46810
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
xbolva00 added inline comments.
Comment at: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:179
+ if (!m_die_array.empty()) {
+lldbassert(!m_first_die || m_first_die == m_die_array.front());
+m_first_die = m_die_array.front();
@jankratochvil is this correct assert? Our downstream lldb based on lldb 7 with
our custom targets hits this assert. If we remove it, we see no obvious
breakages.
Repository:
rL LLVM
https://reviews.llvm.org/D46810
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
jankratochvil marked an inline comment as done. jankratochvil added a comment. FYI I also checked in a regression (just looking at the source code) https://reviews.llvm.org/rL333517. Repository: rL LLVM https://reviews.llvm.org/D46810 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
jankratochvil marked an inline comment as done.
jankratochvil added inline comments.
Comment at: lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:208
+void DWARFUnit::ExtractDIEsEndCheck(lldb::offset_t offset) const {
+ lldb::offset_t next_cu_offset = GetNextCompileUnitOffset();
+
xiaobai wrote:
> This function introduces a warning because `next_cu_offset` is never used
> within this function. Did you intend to use it in the if conditional below?
> It looks like you call `GetNextCompileUnitOffset` there instead of using
> `next_cu_offset`.
Fixed now in rL333449, sorry. Yes, you are right, it remained there from
refactorizations.
GDB is using `-Werror` so I was not used to watch for compiler warnings.
Repository:
rL LLVM
https://reviews.llvm.org/D46810
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
jankratochvil marked an inline comment as done.
jankratochvil added inline comments.
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:86
// Keep a flat array of the DIE for binary lookup by DIE offset
- if (!cu_die_only) {
+ {
Log *log(
clayborg wrote:
> Do we need this empty scope? Remove?
There is now enclosed:
```Log *log(LogChannelDWARF::GetLogIfAny(DWARF_LOG_DEBUG_INFO |
DWARF_LOG_LOOKUPS));
```
While lower (currently moved into `DWARFUnit::ExtractDIEsEndCheck()`) there was:
```Log *log(LogChannelDWARF::GetLogIfAll(DWARF_LOG_DEBUG_INFO));
```
So those two did conflict while being a different variable. I am not sure which
way is right but I have removed the empty scope as it is no longer required.
Then also I am not sure the log in `DWARFUnit::ExtractDIEsEndCheck()` should
not also have `| DWARF_LOG_LOOKUPS` but that would be out of the scope of this
patch topic.
https://reviews.llvm.org/D46810
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Only question is if we remove the extra empty scope as noted in inline
comments. Looks good.
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:86
// Keep a flat array of the DIE for binary lookup by DIE offset
- if (!cu_die_only) {
+ {
Log *log(
Do we need this empty scope? Remove?
https://reviews.llvm.org/D46810
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
jankratochvil updated this revision to Diff 148801.
jankratochvil added a comment.
Split out `ExtractUnitDIEIfNeeded()` out of `ExtractDIEsIfNeeded()`.
https://reviews.llvm.org/D46810
Files:
source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
source/Plugins/SymbolFile/DWARF/DWARFUnit.h
source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
Index: source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -50,9 +50,9 @@
auto extract_fn = [_info, _cu_dies](size_t cu_idx) {
DWARFUnit *dwarf_cu = debug_info.GetCompileUnitAtIndex(cu_idx);
if (dwarf_cu) {
- // dwarf_cu->ExtractDIEsIfNeeded(false) will return zero if the DIEs
+ // dwarf_cu->ExtractDIEsIfNeeded() will return false if the DIEs
// for a compile unit have already been parsed.
- if (dwarf_cu->ExtractDIEsIfNeeded(false) > 1)
+ if (dwarf_cu->ExtractDIEsIfNeeded())
clear_cu_dies[cu_idx] = true;
}
};
Index: source/Plugins/SymbolFile/DWARF/DWARFUnit.h
===
--- source/Plugins/SymbolFile/DWARF/DWARFUnit.h
+++ source/Plugins/SymbolFile/DWARF/DWARFUnit.h
@@ -39,7 +39,8 @@
public:
virtual ~DWARFUnit();
- size_t ExtractDIEsIfNeeded(bool cu_die_only);
+ void ExtractUnitDIEIfNeeded();
+ bool ExtractDIEsIfNeeded();
DWARFDIE LookupAddress(const dw_addr_t address);
size_t AppendDIEsWithTag(const dw_tag_t tag,
DWARFDIECollection _dies,
@@ -160,7 +161,7 @@
dw_offset_t GetBaseObjOffset() const;
die_iterator_range dies() {
-ExtractDIEsIfNeeded(false);
+ExtractDIEsIfNeeded();
return die_iterator_range(m_die_array.begin(), m_die_array.end());
}
@@ -173,6 +174,10 @@
void *m_user_data = nullptr;
// The compile unit debug information entry item
DWARFDebugInfoEntry::collection m_die_array;
+ // GetUnitDIEPtrOnly() needs to return pointer to the first DIE.
+ // But the first element of m_die_array after ExtractUnitDIEIfNeeded()
+ // would possibly move in memory after later ExtractDIEsIfNeeded().
+ DWARFDebugInfoEntry m_first_die;
// A table similar to the .debug_aranges table, but this one points to the
// exact DW_TAG_subprogram DIEs
std::unique_ptr m_func_aranges_ap;
@@ -202,21 +207,22 @@
// Get the DWARF unit DWARF debug informration entry. Parse the single DIE
// if needed.
const DWARFDebugInfoEntry *GetUnitDIEPtrOnly() {
-ExtractDIEsIfNeeded(true);
-if (m_die_array.empty())
+ExtractUnitDIEIfNeeded();
+if (!m_first_die)
return NULL;
-return _die_array[0];
+return _first_die;
}
// Get all DWARF debug informration entries. Parse all DIEs if needed.
const DWARFDebugInfoEntry *DIEPtr() {
-ExtractDIEsIfNeeded(false);
+ExtractDIEsIfNeeded();
if (m_die_array.empty())
return NULL;
return _die_array[0];
}
- void AddUnitDIE(DWARFDebugInfoEntry );
+ void AddUnitDIE(const DWARFDebugInfoEntry _die);
+ void ExtractDIEsEndCheck(lldb::offset_t offset) const;
DISALLOW_COPY_AND_ASSIGN(DWARFUnit);
};
Index: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -14,6 +14,7 @@
#include "lldb/Symbol/CompileUnit.h"
#include "lldb/Symbol/LineTable.h"
#include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Utility/LLDBAssert.h"
#include "lldb/Utility/StreamString.h"
#include "lldb/Utility/Timer.h"
@@ -35,28 +36,54 @@
DWARFUnit::~DWARFUnit() {}
//--
-// ParseCompileUnitDIEsIfNeeded
-//
+// Parses first DIE of a compile unit.
+//--
+void DWARFUnit::ExtractUnitDIEIfNeeded() {
+ if (m_first_die)
+return; // Already parsed
+
+ static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
+ Timer scoped_timer(
+ func_cat, "%8.8x: DWARFUnit::ExtractUnitDIEIfNeeded()", m_offset);
+
+ // Set the offset to that of the first DIE and calculate the start of the
+ // next compilation unit header.
+ lldb::offset_t offset = GetFirstDIEOffset();
+
+ // We are in our compile unit, parse starting at the offset we were told to
+ // parse
+ const DWARFDataExtractor = GetData();
+ DWARFFormValue::FixedFormSizes fixed_form_sizes =
+ DWARFFormValue::GetFixedFormSizesForAddressSize(GetAddressByteSize(),
+ IsDWARF64());
+ if (offset < GetNextCompileUnitOffset() &&
+ m_first_die.FastExtract(data, this, fixed_form_sizes, )) {
+AddUnitDIE(m_first_die);
+
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
jankratochvil added inline comments.
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:312
void DWARFUnit::ClearDIEs(bool keep_compile_unit_die) {
+ m_die_array.clear();
labath wrote:
> You are ignoring the keep_compile_unit_die argument here. Is that
> intentional? (I would be fine with not being able to clear the unit die, but
> if we got that way, then we should remove this argument altogether.)
I did not notice, true. But there is no caller of `ClearDIEs(false)` anyway.
And with `m_first_die` it has even no longer any benefit to clear even the
first DIE.
https://reviews.llvm.org/D46810
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
labath added a comment.
+1 for the cleanup. The `m_die_array_size` function is just weird
Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:312
void DWARFUnit::ClearDIEs(bool keep_compile_unit_die) {
+ m_die_array.clear();
You are ignoring the keep_compile_unit_die argument here. Is that intentional?
(I would be fine with not being able to clear the unit die, but if we got that
way, then we should remove this argument altogether.)
https://reviews.llvm.org/D46810
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
clayborg added a comment. In https://reviews.llvm.org/D46810#570, @jankratochvil wrote: > In https://reviews.llvm.org/D46810#1098110, @clayborg wrote: > > > So this problem exists both in the LLDB and LLVM DWARF parsers. I am not > > sure this fix is safe. I would rather fix this by fixing DWARFDIE class to > > "do the right thing". We should be able to teach the DWARFDIE class to > > replace its "m_die" with the updated "m_die" if a method ever causes > > DWARFDIE to need to expand all DIEs in a DWARFUnit. That seems like a much > > safer fix. Having m_first_die is not safe because it if you call > > DWARFDIE::GetFirstChild() it will just add 1 to the "m_die" and we will > > crash. All parent, sibling and child code just do pointer arithmetic to > > find their counterparts. So since DWARFDIE has the "DWARFUnit *m_cu;" and > > "DWARFDebugInfoEntry *m_die;" we should use DWARFDIE to abstract this from > > users. Anyone playing directly with DWARFDebugInfoEntry must know the rules > > and do the right thing or just use DWARFDIE. > > > Is this statement still valid now with `DWARFBaseDIE`? Statement isn't valid, but we should cleanup the DIE parsing code so we have dedicated parsing for the unit DIE only and for all DIEs and remove the m_die_array_size() function. https://reviews.llvm.org/D46810 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
clayborg added a comment.
A better solution here would be to have two functions: one for parsing the Unit
DIE only and one for parsing all DIEs:
class DWARFUnit {
void ExtractUnitDIEIfNeeded();
size_t ExtractDIEsIfNeeded();
}
Then the code becomes much simpler, we don't need the "m_die_array_size"
function and logic is much cleaner,
DWARFUnit::ExtractUnitDIEIfNeeded() will extract into m_first_die only and
won't touch m_die_array at all. DWARFUnit::ExtractDIEsIfNeeded() will extract
all DIEs into m_die_array. Then there is no need for worrying about
m_die_array.size().
https://reviews.llvm.org/D46810
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
jankratochvil added a comment. In https://reviews.llvm.org/D46810#1098110, @clayborg wrote: > So this problem exists both in the LLDB and LLVM DWARF parsers. I am not sure > this fix is safe. I would rather fix this by fixing DWARFDIE class to "do the > right thing". We should be able to teach the DWARFDIE class to replace its > "m_die" with the updated "m_die" if a method ever causes DWARFDIE to need to > expand all DIEs in a DWARFUnit. That seems like a much safer fix. Having > m_first_die is not safe because it if you call DWARFDIE::GetFirstChild() it > will just add 1 to the "m_die" and we will crash. All parent, sibling and > child code just do pointer arithmetic to find their counterparts. So since > DWARFDIE has the "DWARFUnit *m_cu;" and "DWARFDebugInfoEntry *m_die;" we > should use DWARFDIE to abstract this from users. Anyone playing directly with > DWARFDebugInfoEntry must know the rules and do the right thing or just use > DWARFDIE. Is this statement still valid now with `DWARFBaseDIE`? https://reviews.llvm.org/D46810 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D46810: 3/3: Fix DWARFUnit::GetUnitDIEPtrOnly stale pointer
jankratochvil updated this revision to Diff 148480.
https://reviews.llvm.org/D46810
Files:
source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
source/Plugins/SymbolFile/DWARF/DWARFUnit.h
Index: source/Plugins/SymbolFile/DWARF/DWARFUnit.h
===
--- source/Plugins/SymbolFile/DWARF/DWARFUnit.h
+++ source/Plugins/SymbolFile/DWARF/DWARFUnit.h
@@ -173,6 +173,17 @@
void *m_user_data = nullptr;
// The compile unit debug information entry item
DWARFDebugInfoEntry::collection m_die_array;
+ // GetUnitDIEPtrOnly() needs to return pointer to the first DIE.
+ // But the first element of m_die_array after ExtractDIEsIfNeeded(true)
+ // may move in memory after later ExtractDIEsIfNeeded(false).
+ // If nothing has been parsed, m_die_array is empty and m_first_die is empty.
+ // If CU DIE has been parsed, m_die_array is empty and m_first_die is full.
+ // if everything has been parsed m_first_die and m_die_array are full,
+ // and the first element of m_die_array contains a copy of m_first_die.
+ DWARFDebugInfoEntry m_first_die;
+ // m_die_array_size() is like m_die_array.size() but
+ // it considers also m_die_array.empty() with m_first_die.
+ size_t m_die_array_size() const;
// A table similar to the .debug_aranges table, but this one points to the
// exact DW_TAG_subprogram DIEs
std::unique_ptr m_func_aranges_ap;
@@ -203,9 +214,9 @@
// if needed.
const DWARFDebugInfoEntry *GetUnitDIEPtrOnly() {
ExtractDIEsIfNeeded(true);
-if (m_die_array.empty())
+if (!m_first_die)
return NULL;
-return _die_array[0];
+return _first_die;
}
// Get all DWARF debug informration entries. Parse all DIEs if needed.
Index: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
===
--- source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
+++ source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
@@ -40,7 +40,7 @@
// Parses a compile unit and indexes its DIEs if it hasn't already been done.
//--
size_t DWARFUnit::ExtractDIEsIfNeeded(bool cu_die_only) {
- const size_t initial_die_array_size = m_die_array.size();
+ const size_t initial_die_array_size = m_die_array_size();
if ((cu_die_only && initial_die_array_size > 0) || initial_die_array_size > 1)
return 0; // Already parsed
@@ -92,6 +92,22 @@
if (depth == 0) {
if (initial_die_array_size == 0)
AddUnitDIE(die);
+
+ if (!cu_die_only) {
+assert(m_die_array.empty() && "Compile unit DIE already added");
+
+// The average bytes per DIE entry has been seen to be around 14-20 so
+// lets pre-reserve half of that since we are now stripping the NULL
+// tags.
+
+// Only reserve the memory if we are adding children of the main
+// compile unit DIE. The compile unit DIE is always the first entry, so
+// if our size is 1, then we are adding the first compile unit child
+// DIE and should reserve the memory.
+m_die_array.reserve(GetDebugInfoSize() / 24);
+m_die_array.push_back(die);
+ }
+
uint64_t base_addr = die.GetAttributeValueAsAddress(
m_dwarf, this, DW_AT_low_pc, LLDB_INVALID_ADDRESS);
if (base_addr == LLDB_INVALID_ADDRESS)
@@ -101,14 +117,17 @@
if (cu_die_only)
return 1;
} else {
+ assert(!cu_die_only);
if (null_die) {
if (prev_die_had_children) {
// This will only happen if a DIE says is has children but all it
// contains is a NULL tag. Since we are removing the NULL DIEs from
// the list (saves up to 25% in C++ code), we need a way to let the
// DIE know that it actually doesn't have children.
- if (!m_die_array.empty())
+ if (!m_die_array.empty()) {
m_die_array.back().SetEmptyChildren(true);
+m_first_die.SetEmptyChildren(true);
+ }
}
} else {
die.SetParentIndex(m_die_array.size() - die_index_stack[depth - 1]);
@@ -134,7 +153,7 @@
prev_die_had_children = false;
} else {
- die_index_stack.back() = m_die_array.size() - 1;
+ die_index_stack.back() = m_die_array_size() - 1;
// Normal DIE
const bool die_has_children = die.HasChildren();
if (die_has_children) {
@@ -168,36 +187,38 @@
if (log && log->GetVerbose()) {
StreamString strm;
Dump();
-if (m_die_array.empty())
+if (!m_first_die)
strm.Printf("error: no DIE for compile unit");
else
- m_die_array[0].Dump(m_dwarf, this, strm, UINT32_MAX);
+ m_first_die.Dump(m_dwarf, this, strm, UINT32_MAX);
log->PutString(strm.GetString());
}
+ assert(m_die_array.empty() || !cu_die_only);
+
