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 */
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.
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
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
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
elfutils-devel@lists.fedorahosted.org
-
Petr Machata -
Roland McGrath