Re: gcc7 log2 compile issues in kernel/time/timekeeping.c

[Laura Abbott]

On 02/25/2017 03:50 AM, Ard Biesheuvel wrote:
> 
> 
>> On 25 Feb 2017, at 11:23, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
>>
>> On 25 February 2017 at 11:09, Markus Trippelsdorf
>> <markus@xxxxxxxxxxxxxxx> wrote:
>>> On 2017.02.25 at 09:11 +0000, Ard Biesheuvel wrote:
>>>>> On 25 February 2017 at 08:18, Markus Trippelsdorf <markus@xxxxxxxxxxxxxxx> wrote:
>>>>>
>>>>> Why not simply get rid of the ____ilog2_NaN thing altogether?
>>>>>
>>>>
>>>> That would remove the issue, sure. But we lose an opportunity to spot
>>>> incorrect code at compile time.
>>>
>>> In the case of kernel/time/timekeeping.c it is clearly a false positive.
>>> Was ever incorrect code spotted by ____ilog2_NaN in the past?
>>>
>>>> My concern is that it by not pushing back on changes to the semantics
>>>> of __builtin_constant_p() such as this one, we may start seeing other
>>>> issues where we can no longer use it, and we lose a very useful tool.
>>>
>>> We had a long discussion in:
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72785
>>> As you can see there is no real consensus.
>>> But ilog2 seems to be the only place where this ever popped up.
>>> (There were several distro-wide mass rebuilds with gcc-7 and no other
>>> __builtin_constant_p() issue was found yet.)
>>>
>>
>> Well, given that it is really dead code that is being emitted, and
>> that log2(0) is really undefined, perhaps we should simply replace
>> ilog2_NaN() with __builtin_unreachable()?
> 
> ... or perhaps it is better to just pass the constant == 0 to the runtime implementation?
> 
> The second ilog2_NaN is really unreachable, given that it deals with unsigned values >0 without a single bit set.
> 

naively throwing in __builtin_unreachable() doesn't seem to
work:

./include/linux/log2.h: In function ‘__order_base_2’:
./include/linux/log2.h:155:10: error: void value not ignored as it ought to be

I'm guessing unreachable is treated as void instead of all
possible types and therefore gcc assumes that the entire
function must be void?

Thanks,
Laura