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

[Petr Machata <pmachata@redhat.com>]

[-- 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