On 12/12/2013 04:13 AM, Petr Machata wrote:
Josh Stone <jistone(a)redhat.com> writes:
> +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.
Ah, yes, I should explain that a little. I found that the code was
actually faster with the first step unrolled. And you may have noticed
I didn't do this for sleb128, because I found that slightly slower.
It seems to work like an extra hint to the compiler. There's already a
likely-return in the loop, but writing it this way seems to convince gcc
that the first step in particular is *really* likely to return. Then
that step also gets to const-prop the zero.
I can only guess that the sign-extension for sleb128 is complicated
enough to nullify the gain of unrolling.
> +#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.
Right, there are two reasons I kept it this way. One is the cast for
set-but-not-used, as you note, which didn't trigger before because the
former macros did both read and write the value in place. I think there
was only one caller affected though, so I could just change that. The
do-while keeps the macro as a statement, vs your expression, but maybe
that doesn't matter much.
> +#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!
I should give credit, I found that trick here:
http://graphics.stanford.edu/~seander/bithacks.html#FixedSignExtend
The former code was trying to sign-extend after the value was shifted
and combined, which as a variable width is harder. I really like that
this way the compiler is fully aware that this is a sign extension,
rather than being a side effect of ORing bits or left-right shifts.
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.
I agree the original was convoluted, and I think it was actually broken
for some edge cases, though you almost have to look at it preprocessed
to make sense of it. In particular, __libdw_get_sleb128 ORed with acc
in the final step, but only _v was kept updated throughout the loop.
(But it's unlikely any real DWARF had a 10-byte sleb128 to hit this.)