On 10/12/15 17:04, Benjamin Kaduk wrote:
> On 12/10/2015 05:55 AM, Jayalakshmi bhat wrote:
>> Hi Matt,
>>
>> Thanks for the patch. Unfortunately patch did not work. I continued
>> debugging and found that issue was in constant_time_msb.
>>
>> static inline unsigned int constant_time_msb(unsigned int a) {
>> - *return 0 - (a >> (sizeof(a) * 8 - 1));*
>> + return (((unsigned)((int)(a) >> (sizeof(int) * 8 - 1))));
>
> Hmm, right-shifting a negative value is implementation-defined behavior,
> so I don't think that this construct would necessarily be portable to
> all systems. It's not clear to me what purpose the "0 - " was supposed
> to perform in the original version, though.
This function is supposed to copy the msb of the input to all of the
other bits...so the return value should either be one of 0x00000000 or
0xffffffff (obviously dependent on how big an int is). In the original
version the shift would give you just the msb, i.e. a value of 0 or 1.
After the "0 -" you get 0 or -1 (0xffffffff).
Interestingly I just checked the header file where this function is
defined and found this:
/*
* Returns the given value with the MSB copied to all the other
* bits. Uses the fact that arithmetic shift shifts-in the sign bit.
* However, this is not ensured by the C standard so you may need to
* replace this with something else on odd CPUs.
*/
This doesn't match up with the implementation - so I suspect the comment
predates the implementation - but Jaya's fix would put it back that way.
Matt
