[PATCH 2/3] Simplify and inline get_uleb128 and get_sleb128
Petr Machata
pmachata at redhat.com
Thu Dec 12 12:13:06 UTC 2013
Josh Stone <jistone at redhat.com> writes:
> diff --git a/libdw/ChangeLog b/libdw/ChangeLog
> index a2e4b142a107..93396e404d97 100644
> --- a/libdw/ChangeLog
> +++ b/libdw/ChangeLog
> @@ -1,3 +1,17 @@
> +2013-12-10 Josh Stone <jistone at redhat.com>
> +
> + * memory-access.h (get_uleb128_rest_return): Removed.
> + (get_sleb128_rest_return): Removed
Period.
> +static inline uint64_t
> +__libdw_get_uleb128 (const unsigned char **addrp)
> +{
> + uint64_t acc = 0;
> + get_uleb128_step (acc, *addrp, 0);
> + for (unsigned int i = 1; i < len_leb128(acc); ++i)
> + get_uleb128_step (acc, *addrp, i);
Is there a reason not to use for (i = 0; ...) instead of the pre-step
followed by a for (i = 1; ...)? The only difference would be for
len_leb128 of 0, where your code makes one step anyway, but that can't
happen. Then you could just expand get_uleb128_step inline, and get rid
of it altogether.
Also, the len_leb128 call needs space before (...).
> + /* Other implementations set VALUE to UINT_MAX in this
> + case. So we better do this as well. */
> + return UINT64_MAX;
> +}
> +
> +#define get_uleb128(var, addr) \
> do { \
> + (var) = __libdw_get_uleb128(&(addr)); \
> + (void)(var); \
> } while (0)
Space before (...). Space after type cast operator.
Actually, could this actually be defined like this?
#define get_uleb128(var, addr) ((var) = __libdw_get_uleb128 (&(addr)))
Then you wouldn't need the cast as the only set-but-not-used warnings
would come from sites that actually set but not use, and those could be
changed to simply call __libdw_get_uleb128. But that's probably a
logically separate change.
> +#define get_sleb128_step(var, addr, nth) \
> do { \
> + unsigned char __b = *(addr)++; \
> + if (likely ((__b & 0x80) == 0)) \
> { \
> + struct { signed int i:7; } __s = { .i = __b }; \
> + (var) |= (typeof (var)) __s.i << ((nth) * 7); \
Oh, the bitfield trick is clever!
> + return (var); \
> } \
> + (var) |= (typeof (var)) (__b & 0x7f) << ((nth) * 7); \
> } while (0)
>
> static inline int64_t
> +__libdw_get_sleb128 (const unsigned char **addrp)
> {
> + int64_t acc = 0;
> + for (unsigned int i = 0; i < len_leb128(acc); ++i)
Space before (...).
> + get_sleb128_step (acc, *addrp, i);
> + /* Other implementations set VALUE to INT_MAX in this
> + case. So we better do this as well. */
> + return INT64_MAX;
> }
> +
> +#define get_sleb128(var, addr) \
> + do { \
> + (var) = __libdw_get_sleb128(&(addr)); \
> + (void)(var); \
> + } while (0)
Same comments as for get_uleb128.
This all looks fine to me. I find the new code much clearer than the
original, which seemed to wind every which way and was hard to follow.
Thanks,
PM
More information about the elfutils-devel
mailing list