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