The scenario goes like this. * end iterator has an offset of 1 * begin iterator has an offset of 0 * operator ++ does essentially the following: _m_offset = ::dwarf_getattrs (&_m_die._m_die, &getattrs_callback, (void *) this, _m_offset); * operator == takes into account "parental" DIE and _m_offset
getattrs_callback is done such that it answers CB_OK when called the first time (then it also initializes _m_attr.value), and CB_ABORT when called the second time. This way dwarf_getattrs returns the position of _next_ attribute in sequence. But that means that when the last argument is loaded in _m_attr, _m_offset already has a value of 1, and evaluates as equal to the end iterator.
As a fix for that, I propose to consider also NULL-ness of _m_attr.value when doing the iterator comparison. On empty sequences, the callback is never called, thus leaving _m_attr.value NULL. On sequence of one element, _m_offset will end up being 1, but _m_attr.value will be non-NULL. Only after the call to ++ will _m_attr.value become NULL, and dwarf_getattrs, seeing offset of 1, will immediately return 1 again without calling the callback. Etc.
Patch attached.
PM
From 3e9c7e605e666ef32d1dddd93f9d303ebd801e67 Mon Sep 17 00:00:00 2001 From: Petr Machata pmachata@redhat.com Date: Tue, 24 Mar 2009 15:04:53 +0100 Subject: [PATCH] <dwarf>: Don't swallow last attribute
--- libdw/ChangeLog | 5 +++++ libdw/c++/dwarf | 11 ++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/libdw/ChangeLog b/libdw/ChangeLog index fc08176..d68075d 100644 --- a/libdw/ChangeLog +++ b/libdw/ChangeLog @@ -1,3 +1,8 @@ +2009-03-24 Petr Machata pmachata@redhat.com + + * c++/dwarf (dwarf::debug_info_entry::raw_attributes): + Fix iteration over attributes. + 2009-02-26 Roland McGrath roland@redhat.com
* c++/dwarf (dwarf::attr_value): Add _m_tag private member. diff --git a/libdw/c++/dwarf b/libdw/c++/dwarf index 862b9a1..33e7250 100644 --- a/libdw/c++/dwarf +++ b/libdw/c++/dwarf @@ -626,7 +626,10 @@ namespace elfutils }
inline const_iterator (const debug_info_entry &die, ptrdiff_t offset) - : _m_die (die), _m_offset (offset) {} + : _m_die (die), _m_offset (offset) + { + _m_attr.valp = NULL; + }
public: inline const_iterator (const const_iterator &i) @@ -643,7 +646,8 @@ namespace elfutils inline bool operator== (const const_iterator &other) const { return (_m_die._m_die.addr == other._m_die._m_die.addr - && _m_offset == other._m_offset); + && _m_offset == other._m_offset + && !_m_attr.valp == !other._m_attr.valp); } inline bool operator!= (const const_iterator &other) const { @@ -659,6 +663,7 @@ namespace elfutils _m_offset = result; return *this; } + inline const_iterator operator++ (int) // postfix { const_iterator prev = *this; @@ -668,7 +673,7 @@ namespace elfutils
inline attribute operator* () const { - if (unlikely (_m_offset == 1)) + if (unlikely (_m_offset == 1 && _m_attr.valp == NULL)) throw std::runtime_error ("dereferencing end iterator"); return attribute (_m_die, _m_attr); }
As a general thing, please also push to some private branch when you post. (One-offs are good for this, you can remove them later with push :user/foo.) That way approvers can merge it directly rather than either telling you to push it or using git-am. (In this case, I think I want a different version merged instead. But this is a note for general procedure.)
The scenario goes like this.
- end iterator has an offset of 1
- begin iterator has an offset of 0
Actually that's the pre-begin iterator.
inline const_iterator begin () const { const_iterator i = const_iterator (_m_die, 0); return ++i; }
The initialized-to-0 iterator is never seen anywhere else. Hence:
inline const_iterator (const debug_info_entry &die, ptrdiff_t offset)
: _m_die (die), _m_offset (offset) {}
: _m_die (die), _m_offset (offset)- {
_m_attr.valp = NULL;- }
No need for that, given:
inline const_iterator &operator++ () // prefix { _m_attr.valp = NULL; ...
As a fix for that, I propose to consider also NULL-ness of _m_attr.value when doing the iterator comparison.
I think you can just do valp == valp comparison and drop the offset comparison. The pointers will match when the offsets match, and two NULLs are two end iterators.
Can you also please write a test case for this? I'd guess a trivial program using .to_string () does it since the busted iterator is used by that.
I've just pushed makefile cleanups that should make it simpler to copy the src/Makefile.am magic into tests/Makefile.am to compile C++ cases there.
Thanks, Roland
Roland McGrath wrote:
As a fix for that, I propose to consider also NULL-ness of _m_attr.value when doing the iterator comparison.
I think you can just do valp == valp comparison and drop the offset comparison. The pointers will match when the offsets match, and two NULLs are two end iterators.
Can you also please write a test case for this? I'd guess a trivial program using .to_string () does it since the busted iterator is used by that.
The changes that you requested, and the testcase, are on pmachata/dwarf.
PM
The changes that you requested, and the testcase, are on pmachata/dwarf.
Thanks.
@@ -637,7 +637,7 @@ namespace elfutils
inline attribute operator* () const { - if (unlikely (_m_offset == 1)) + if (unlikely (_m_offset == 1 && _m_attr.valp == NULL)) throw std::runtime_error ("dereferencing end iterator"); return attribute (_m_die, _m_attr); }
We don't really need the _m_offset check here, do we?
Why do we need the new dwarf-attributes test program instead of testing the expected output of dwarf-print? I guess its output is kind of long on any nontrivial file. Perhaps just tweak dwarf-print with an optional less verbose mode rather than cut&paste most of it?
Thanks, Roland
Roland McGrath wrote:
The changes that you requested, and the testcase, are on pmachata/dwarf.
Thanks.
@@ -637,7 +637,7 @@ namespace elfutils
inline attribute operator* () const {
if (unlikely (_m_offset == 1))
if (unlikely (_m_offset == 1 && _m_attr.valp == NULL)) throw std::runtime_error ("dereferencing end iterator");We don't really need the _m_offset check here, do we?
We do, that's the new end iterator condition. offset==1 valp!=NULL is valid iterator pointing to the last element.
Why do we need the new dwarf-attributes test program instead of testing the expected output of dwarf-print? I guess its output is kind of long on any nontrivial file. Perhaps just tweak dwarf-print with an optional less verbose mode rather than cut&paste most of it?
To test the fix, all that is necessary is to pick the first CU DIE and iterate its attributes, and that's what the test does. The goal is for the test to remain valid as long as the tested condition holds. dwarf-print output will change in all kinds of ways as we change the library. But I pushed another way of doing this that doesn't involve duplicate code, and does involve sed, which is my favourite combination.
PM
We do, that's the new end iterator condition. offset==1 valp!=NULL is valid iterator pointing to the last element.
But valp==NULL is never a valid non-end iterator.
To test the fix, all that is necessary is to pick the first CU DIE and iterate its attributes, and that's what the test does. The goal is for the test to remain valid as long as the tested condition holds. dwarf-print output will change in all kinds of ways as we change the library. But I pushed another way of doing this that doesn't involve duplicate code, and does involve sed, which is my favourite combination.
sed only gets points when it's third-order or better (sed producing sed programs that produce sed programs). ;-)
I'm not sure $srcdir/ works right with testrun for installcheck et al. And we don't need more tiny wrapper scripts really. I added --depth=1 to dwarf-print.
Thanks, Roland
Petr Machata wrote:
Roland McGrath wrote:
We do, that's the new end iterator condition. offset==1 valp!=NULL is valid iterator pointing to the last element.
But valp==NULL is never a valid non-end iterator.
Ah, I didn't notice you talk about offset. Right, we don't need offset check.
It's on my branch.
PM
elfutils-devel@lists.fedorahosted.org
-
Petr Machata -
Roland McGrath