Re: [PATCH] Support 1-sized reads in read_ubyte_unaligned_inc and read_sbyte_unaligned_inc

Re: [PATCH] Support 1-sized reads in read_ubyte_unaligned_inc and read_sbyte_unaligned_inc

[Petr Machata]

[-- Attachment #1: Type: text/plain, Size: 2591 bytes --]

Roland McGrath <roland@hack.frob.com> writes:

>> Five, four of them with constant size and one with a 64bit?8:4 sort of
>> expression.  The reads are done through a macro that checks bounds,
>> There's one macro for all the widths, mostly because I didn't like to
>> have four macros with unknown cut'n'paste errors.  I expect that the
>> compiler will be able to see through and just inline a check and an
>> access for the right width directly, but I didn't actually check.
>
> The existing uses are in practice 64bit?8:4 cases.  They just happen
> to allow a nonsensical 2 case rather than diagnosing it (i.e. a
> crazy FDE encoding) early.  readelf.c's encoded_ptr_size has exactly
> this one caller, so it could just be rolled in there and have a
> diagnostic and early bail for anything that's not a 4- or 8-wide
> encoding.  Then we could have a memory-access.h macro that is
> specifically only for "ptr_size" (4 or 8), and using the single-size
> named macros for the constant-size cases.

Right, that would be meaningful.

> For your macro used for constant-size cases, you could make it a
> single macro that takes the size literal as a macro argument and
> uses read_##size##ubyte_unaligned_inc.

Sure, I can do that, and add the corresponding _1ubyte/_1sbyte {,inc}
calls if you find that more acceptable.

>> Admittedly this is all somewhat moot.  I don't check bounds with LEB's
>> anyway, and most of libdw just checks post fact that the pointers are
>> still in bounds.  Maybe I should simply do the same.
>
> We should be consistent throughout the codebase, one way or another.
> If the bounds-checking matters for one case, it matters for the
> others; if we don't care for umpteen cases, we shouldn't care any
> more for one more case.  I've never been happy with the lossy bounds
> checking, and tend to think we should fix it up all throughout.

Bounds checking of course matters for all cases the same.  But some have
bugs in them.  That's no reason to add more buggy code, even if it
happens to be consistently buggy.  To put it differently, I don't see
value--on side of maintainer or user--to have the guarantee that we
don't do any bounds checking.

But I realized something else.  A patch that implements .debug_macro
support shouldn't happen to add bounds-checking primitives in a
by-the-way manner like mine does, and moreover keep them private.  That
should be a separate patch adding this stuff in a properly global
manner, with a separate discussion and all that.

So yeah, let me rework it.

Thanks,
PM

Re: [PATCH] Support 1-sized reads in read_ubyte_unaligned_inc and read_sbyte_unaligned_inc

[Roland McGrath]

[-- Attachment #1: Type: text/plain, Size: 2129 bytes --]

> Five, four of them with constant size and one with a 64bit?8:4 sort of
> expression.  The reads are done through a macro that checks bounds,
> There's one macro for all the widths, mostly because I didn't like to
> have four macros with unknown cut'n'paste errors.  I expect that the
> compiler will be able to see through and just inline a check and an
> access for the right width directly, but I didn't actually check.

The existing uses are in practice 64bit?8:4 cases.  They just happen
to allow a nonsensical 2 case rather than diagnosing it (i.e. a
crazy FDE encoding) early.  readelf.c's encoded_ptr_size has exactly
this one caller, so it could just be rolled in there and have a
diagnostic and early bail for anything that's not a 4- or 8-wide
encoding.  Then we could have a memory-access.h macro that is
specifically only for "ptr_size" (4 or 8), and using the single-size
named macros for the constant-size cases.

For your macro used for constant-size cases, you could make it a
single macro that takes the size literal as a macro argument and
uses read_##size##ubyte_unaligned_inc.

> Admittedly this is all somewhat moot.  I don't check bounds with LEB's
> anyway, and most of libdw just checks post fact that the pointers are
> still in bounds.  Maybe I should simply do the same.

We should be consistent throughout the codebase, one way or another.
If the bounds-checking matters for one case, it matters for the
others; if we don't care for umpteen cases, we shouldn't care any
more for one more case.  I've never been happy with the lossy bounds
checking, and tend to think we should fix it up all throughout.  But
that is quite unrelated to your current work.  It probably makes
sense to finish the new thing you're doing now just by following the
existing models, i.e. doing no better on bounds-checking but
matching the idioms we see elsewhere in the code.  Then later (right
away if you care to take it on, in the fullness of time if not) we
can figure out how to revamp the whole memory-access.h interface and
all its users to be fully robust.


Thanks,
Roland

Re: [PATCH] Support 1-sized reads in read_ubyte_unaligned_inc and read_sbyte_unaligned_inc

[Petr Machata]

[-- Attachment #1: Type: text/plain, Size: 2061 bytes --]

Roland McGrath <roland@hack.frob.com> writes:

> But, in actual fact, there are zero uses of read_sbyte_unaligned_inc and
> two uses of read_ubyte_unaligned_inc (they are adjacent in
> readelf.c:print_debug_frame_section).  In both of those uses the size
> argument is a non-constant, so adding another case has runtime cost.
> Clearly that cost doesn't actually matter much in readelf, but it calls
> into question the wisdom of expanding these to more uses without
> considering their performance in those uses.

The performance implications did cross my mind, and I decided that it's
fine in readelf.  (I did check the uses.)

> It also seems questionable from a source-comprehensibility perspective to
> make those support a different set of sizes than read_sbyte_unaligned and
> read_ubyte_unaligned do.  But those have no uses at all, so perhaps they
> should just be removed anyway.

Actually I didn't notice the two other macros.  I agree that such
inconsistency is bad.

That read_sbyte_unaligned_inc should be dropped did actually come to my
mind.  Clearly it's untested and untestable as things currently stand.

> How many new uses are you adding, and what do they look like?  I guess they
> must have non-constant size parameters too, or you would have just used the
> size-specific macros.  So where does the size parameter come from that it
> can have any of the four possible sizes?

Five, four of them with constant size and one with a 64bit?8:4 sort of
expression.  The reads are done through a macro that checks bounds,
There's one macro for all the widths, mostly because I didn't like to
have four macros with unknown cut'n'paste errors.  I expect that the
compiler will be able to see through and just inline a check and an
access for the right width directly, but I didn't actually check.

Admittedly this is all somewhat moot.  I don't check bounds with LEB's
anyway, and most of libdw just checks post fact that the pointers are
still in bounds.  Maybe I should simply do the same.

Thanks,
PM

Re: [PATCH] Support 1-sized reads in read_ubyte_unaligned_inc and read_sbyte_unaligned_inc

[Roland McGrath]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

It sounds reasonable in the abstract.  It looks OK in the implementation,
except you should put parens around the macro arg when used that way.  

But, in actual fact, there are zero uses of read_sbyte_unaligned_inc and
two uses of read_ubyte_unaligned_inc (they are adjacent in
readelf.c:print_debug_frame_section).  In both of those uses the size
argument is a non-constant, so adding another case has runtime cost.
Clearly that cost doesn't actually matter much in readelf, but it calls
into question the wisdom of expanding these to more uses without
considering their performance in those uses.

It also seems questionable from a source-comprehensibility perspective to
make those support a different set of sizes than read_sbyte_unaligned and
read_ubyte_unaligned do.  But those have no uses at all, so perhaps they
should just be removed anyway.

How many new uses are you adding, and what do they look like?  I guess they
must have non-constant size parameters too, or you would have just used the
size-specific macros.  So where does the size parameter come from that it
can have any of the four possible sizes?

The existing pair of uses is in a place where 2 is not even possible in
reality, though it looks like it is possible in theoretical malformed data.

[PATCH] Support 1-sized reads in read_ubyte_unaligned_inc and read_sbyte_unaligned_inc

[Petr Machata]

[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]

I propose this change mostly for consistency.  Having one entry point
for reading all (reasonable) widths is very convenient, and I make use
of this interface in my upcoming patch for .debug_macro support.

---8<-----------------------------------------------------------------
Signed-off-by: Petr Machata <pmachata@redhat.com>
---
 libdw/ChangeLog       |    5 +++++
 libdw/memory-access.h |   10 ++++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 5f9b097..b8c5acd 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,8 @@
+2014-09-10  Petr Machata  <pmachata@redhat.com>
+
+	* memory-access.h (read_ubyte_unaligned_inc)
+	(read_sbyte_unaligned_inc): Read 1-sized fields as well.
+
 2014-08-15  Mark Wielaard  <mjw@redhat.com>
 
 	* dwarf_cu_die.c: New file.
diff --git a/libdw/memory-access.h b/libdw/memory-access.h
index f41f783..caa92f6 100644
--- a/libdw/memory-access.h
+++ b/libdw/memory-access.h
@@ -259,13 +259,15 @@ read_8sbyte_unaligned_1 (bool other_byte_order, const void *p)
 
 
 #define read_ubyte_unaligned_inc(Nbytes, Dbg, Addr) \
-  ((Nbytes) == 2 ? read_2ubyte_unaligned_inc (Dbg, Addr)		      \
-   : (Nbytes) == 4 ? read_4ubyte_unaligned_inc (Dbg, Addr)		      \
+  ((Nbytes) == 1 ? (uint8_t) *Addr++					\
+   : (Nbytes) == 2 ? read_2ubyte_unaligned_inc (Dbg, Addr)		\
+   : (Nbytes) == 4 ? read_4ubyte_unaligned_inc (Dbg, Addr)		\
    : read_8ubyte_unaligned_inc (Dbg, Addr))
 
 #define read_sbyte_unaligned_inc(Nbytes, Dbg, Addr) \
-  ((Nbytes) == 2 ? read_2sbyte_unaligned_inc (Dbg, Addr)		      \
-   : (Nbytes) == 4 ? read_4sbyte_unaligned_inc (Dbg, Addr)		      \
+  ((Nbytes) == 1 ? (int8_t) *Addr++					\
+   : (Nbytes) == 2 ? read_2sbyte_unaligned_inc (Dbg, Addr)		\
+   : (Nbytes) == 4 ? read_4sbyte_unaligned_inc (Dbg, Addr)		\
    : read_8sbyte_unaligned_inc (Dbg, Addr))
 
 #endif	/* memory-access.h */
-- 
1.7.6.5