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

[Josh Stone <jistone at redhat dot com>]

On 04/23/2014 11:51 AM, Richard Henderson wrote:
> On 04/23/2014 11:32 AM, Petr Machata wrote:
>> Richard Henderson <rth@redhat.com> writes:
>>
>>> On 04/23/2014 03:17 AM, Petr Machata wrote:
>>>> Wouldn't something like this get us off the hook as well?
>>>>
>>>> -	(var) |= (typeof (var)) __s.i << ((nth) * 7);			      \
>>>> +	(var) |= (typeof (var))						      \
>>>> +	  (((uint64_t) (typeof (var)) __s.i) << ((nth) * 7));		      \
>>>>
>>>> We are really only using the bitfield trick to avoid having to
>>>> sign-extend by hand, but we can shift unsigned without losing anything.
>>>
>>> It gets us off the hook, but might introduce a 64-bit shift where
>>> only a 32-bit shift was required.
>>
>> Good point, but get_sleb128_step is only used from __libdw_get_sleb128,
>> where the type is int64_t.  This macro is not safe for outside use
>> anyway, as it uses its parameters more than once.
>>
>> Hmm, should we maybe #undef it after __libdw_get_sleb128, so that it's
>> clear that it's for local use only?
> 
> In that case, why play around with the typeof at all?  Just merge the macro
> into its only user.  At which point using uint64_t makes total sense in context.
> 
> Similarly with get_uleb128_step, I assume, at least wrt to the typeof.

The way these are macro'd is partly a legacy of earlier code.  It does
help for the uleb case since unrolling the first step proved to benefit
optimization.  Keeping sleb this way is helpful to maintain their
similarity as much as possible.

Sure, the typeof's are probably unnecessary anymore, but this is
splitting hairs IMO.  It doesn't help anything to drop this, and we
could decide to provide narrower _libdw_get... in the future for certain
callers.