This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [PATCH 2/3] Simplify and inline get_uleb128 and get_sleb128
- From: Josh Stone <jistone at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Tue, 22 Apr 2014 08:52:01 -0700
- Subject: Re: [PATCH 2/3] Simplify and inline get_uleb128 and get_sleb128
On 04/22/2014 08:01 AM, Richard Henderson wrote:
> On 04/22/2014 07:55 AM, Mark Wielaard wrote:
>> On Thu, 2013-12-12 at 11:06 -0800, Josh Stone wrote:
>>> On 12/12/2013 04:13 AM, Petr Machata wrote:
>>>> Josh Stone <jistone@redhat.com> writes:
>>>>> +#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.
>>
>> Sadly the neat trick triggers undefined behavior since we are trying to
>> left shift a negative value. Even though it appears to work currently I
>> am slightly afraid a compiler optimization might take advantage of this
>> in the future (since it is undefined behavior it could just assume
>> negative values won't occur) especially since this code is inlined in a
>> lot of places, causing hard to diagnose errors.
>>
>> The attached patch is very much not clever, but does what is intended in
>> a well-defined way (it is basically what the DWARF spec gives as pseudo
>> code). Does anybody see a better way?
>
> Multiply instead of shift. I.e.
>
> (typeof (var)) (__b & 0x7f) * (1 << ((nth) * 7));
>
> With any luck the compiler will even notice it's a multiply by a value with one
> bit set, and convert it back to a shift during optimization.
The trick in the original was that "(typeof (var)) __s.i" would extend
the sign from the 7th bit, which I don't your mask will do. But if the
only UB is left-shifting negative, then I think we can combine them.
> Bah. That 1 needs to be typeof (var) too.
So in total:
struct { signed int i:7; } __s = { .i = __b };
(var) |= (typeof (var)) __s.i * ((typeof (var)) 1 << ((nth) * 7));
Better?