Re: [PATCH 2/3] Simplify and inline get_uleb128 and get_sleb128

[Petr Machata <pmachata@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3057 bytes --]

Josh Stone <jistone@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@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