Re: pmachata/reader_hooks review

[Roland McGrath <roland at redhat dot com>]

I did a quick fresh review of 8b1aad2.

Two cosmetic things still need fixed: Use a space in cast syntax.
To refer to local vars in comments, /* Here FOO is used.  */ rather
than /* Here `foo' is used.  */

Also, in libdwP.h non-defining decls, put the function name on the same
line as the type.  Three cosmetic things.

__libdw_read_begin_end_pair_inc inverts the return value check on
READ_AND_RELOCATE, no?  Since that's the only place that looks at
the value of the expression, just make its value a "was relocated"
Boolean (i.e. "status > 1").

I don't understand the reason for the uintptr_t cast in READ_AND_RELOCATE.
The hooks can just take a void * argument, no need for any casts.

I think we can just drop libdw_readhooks.c entirely for now.  Either make
the two no-op stubs inlines in libdwP.h, or just tweak READ_AND_RELOCATE to
comment out the use of RELOC_HOOK.  (The inlines are better if there are
any callers' arguments or locals that are unused without the RELOC_HOOK
call.)  I guess you'll want stubs there to do the testing anyhow, the
inlines should do fine.  (When the real hooks go in, they will be in
libdw/relocate.c along with the innards of the reloc-aware logic.  Before
that we want to merge onto the trunk with stub inlines that compile away.)

dwarf_formstring could use __libdw_formptr.

dwarf_getaranges and dwarf_getpubnames both had sanity checks on the
offsets like "offset + 3 > d_size" (or 4).  Without these, a bogus offset
could make them read off the end of the buffer in their fixed-size header
reading bit.  Make read_offset take a "minimum to read" argument (pass 0 or
1 in the other places), so the consolidated check for bogus offsets can
calculate that in.  Be sure those checks are immune from integer/pointer
arithmetic overflow from bogus offsets, like:
	if (unlikely (offset > d_size) || unlikely (d_size - offset < minread))

These final nits are small and should be quick to take care of.
It's excellent work all together.  Great job!
Are you about ready to move on to the testing regime?


Thanks,
Roland