From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============3237923205816381129==" MIME-Version: 1.0 From: Petr Machata To: elfutils-devel@lists.fedorahosted.org Subject: Re: [PATCH] Support 1-sized reads in read_ubyte_unaligned_inc and read_sbyte_unaligned_inc Date: Thu, 11 Sep 2014 01:46:10 +0200 Message-ID: In-Reply-To: 20140910212207.4D23F2C39D0@topped-with-meat.com --===============3237923205816381129== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Roland McGrath 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 th= ey > must have non-constant size parameters too, or you would have just used t= he > 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 --===============3237923205816381129==--