Hi Petr,
Looks good. The explicit checking against some TAGs was indeed fragile
and not really checking what we wanted. Some small nitpicks below.
On Wed, 2015-04-01 at 22:05 +0200, Petr Machata wrote:
+2015-04-01 Petr Machata <pmachata(a)redhat.com>
+
+ * libdwP.h (DWARF_E_NOT_CUDIE): New enumerator.
+ (is_cudie): New function.
+ * dwarf_error.c (errmsgs): Add message for DWARF_E_NOT_CUDIE.
+ * dwarf_getsrcfiles.c (dwarf_getsrcfiles): Call is_cudie instead
+ of white-listing valid tags.
+ * dwarf_getsrclines.c (dwarf_getsrclines.c): Likewise.
Note that there is an extra .c in the last line. Should be
"(dwarf_getsrclines)".
--- a/libdw/dwarf_getsrcfiles.c
+++ b/libdw/dwarf_getsrcfiles.c
@@ -39,10 +39,11 @@
int
dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files, size_t *nfiles)
{
- if (unlikely (cudie == NULL
- || (INTUSE(dwarf_tag) (cudie) != DW_TAG_compile_unit
- && INTUSE(dwarf_tag) (cudie) != DW_TAG_partial_unit)))
- return -1;
+ if (cudie == NULL || ! is_cudie (cudie))
+ {
+ __libdw_seterrno (DWARF_E_NOT_CUDIE);
+ return -1;
+ }
The convention in most libdw functions is to signal error on NULL input
(return -1), but not set dw errno. That assumes people "chain" function
calls when getting a DIE and using it. When calling a function to get a
DIE pointer NULL is returned and dw errno set on error. So that a next
call that sees a NULL DIE knows to bail out early, return -1, but not
also sets dw errno. The caller can then just get the original dw errno
from the last call that failed. So I would split this in two checks.
int
dwarf_getsrclines (Dwarf_Die *cudie, Dwarf_Lines **lines, size_t *nlines)
{
- if (unlikely (cudie == NULL
- || (INTUSE(dwarf_tag) (cudie) != DW_TAG_compile_unit
- && INTUSE(dwarf_tag) (cudie) != DW_TAG_partial_unit)))
- return -1;
+ if (cudie == NULL || ! is_cudie (cudie))
+ {
+ __libdw_seterrno (DWARF_E_NOT_CUDIE);
+ return -1;
+ }
Likewise.
OK, with those changes.
Thanks,
Mark