Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Mon, 27 Jul 2015 00:23:36 +0300
From: Alexander Cherepanov <ch3root@...nwall.com>
To: musl@...ts.openwall.com
Subject: Re: Left-shift of negative number

On 2015-07-26 19:53, Rich Felker wrote:
> On Sat, Jul 25, 2015 at 09:26:34PM +0300, Alexander Cherepanov wrote:
>>> I've gone ahead and made the change as commit
>>> fe7582f4f92152ab60e9523bf146fe28ceae51f6. If anything looks wrong,
>>> please let me know. Thanks again for the bug report.
>>
>> The new definition of R:
>>
>> #define R(a,b) ((uint32_t)((a==0x80 ? 0x40u-b : 0u-a) << 23))
>>
>> It implicitly casts a and b to unsigned (and triggers
>> -Wsign-conversion). Isn't it better to express it explicitly, e.g.
>
> Using implicit conversion to unsigned types is idiomatic in musl; we

Sorry, I didn't know it.

> don't use -Wsign-comparison or similar in the default CFLAGS. A
> discussion about changing that should be library-global, not specific
> to this one piece of code, but I'd rather not anyway. My view is that
> there's a tradeoff between using implicit conversions and casts: using
> the implicit conversions (and leaving off warnings for them) could
> hide bugs from wrong expectations about types, but using casts can
> hide much more dangerous conversions for which an implicit conversion
> would not even be possible. I generally like to keep the number of
> casts to a minimum.

It would be interesting to read when and if you have time to write about 
it in more details.

>> by moving the cast to uint32_t inside the conditional operator? Or
>> maybe more intuitive to move the work with negative numbers outside
>> the conditional operator:
>>
>> #define R(a,b) (-(uint32_t)(a==0x80 ? b-0x40 : a) << 23)
>
> This actually would give the wrong result -- and strictly speaking,
> even worse, undefined behavior -- if uint32_t had lower rank than int.
> While other parts of musl assume 32-bit int for Linux-specific reasons
> (advanced parts of the futex API, etc.) I'd rather leave this code
> only assuming that int is >=32bit rather than exactly 32-bit.

Ok, thanks for the explanation.

-- 
Alexander Cherepanov

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.