Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1587949 > unrolled thread

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

Started byJohn Stultz <john.stultz@linaro.org>
First post2017-02-24 22:30 +0100
Last post2017-03-01 02:20 +0100
Articles 9 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: gcc7 log2 compile issues in kernel/time/timekeeping.c John Stultz <john.stultz@linaro.org> - 2017-02-24 22:30 +0100
    Re: gcc7 log2 compile issues in kernel/time/timekeeping.c Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-02-24 22:50 +0100
      Re: gcc7 log2 compile issues in kernel/time/timekeeping.c Laura Abbott <labbott@redhat.com> - 2017-02-25 00:40 +0100
        Re: gcc7 log2 compile issues in kernel/time/timekeeping.c Markus Trippelsdorf <markus@trippelsdorf.de> - 2017-02-25 09:20 +0100
          Re: gcc7 log2 compile issues in kernel/time/timekeeping.c Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-02-25 10:20 +0100
            Re: gcc7 log2 compile issues in kernel/time/timekeeping.c Markus Trippelsdorf <markus@trippelsdorf.de> - 2017-02-25 12:20 +0100
              Re: gcc7 log2 compile issues in kernel/time/timekeeping.c Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-02-25 12:40 +0100
                Re: gcc7 log2 compile issues in kernel/time/timekeeping.c Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-02-25 13:00 +0100
                  Re: gcc7 log2 compile issues in kernel/time/timekeeping.c Laura Abbott <labbott@redhat.com> - 2017-03-01 02:20 +0100

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

FromJohn Stultz <john.stultz@linaro.org>
Date2017-02-24 22:30 +0100
SubjectRe: gcc7 log2 compile issues in kernel/time/timekeeping.c
Message-ID<tevB7-65j-1@gated-at.bofh.it>
On Thu, Feb 23, 2017 at 10:43 AM, Laura Abbott <labbott@redhat.com> wrote:
> Hi,
>
> Fedora was previously carrying a workaround for a gcc7 issue reported
> on arm64 http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/461597.html.
> The workaround got rid of __ilog2_NaN. I dropped the patch this morning
> because a proper fix (29905b52fad0 ("log2: make order_base_2() behave
> correctly on const input value zero")) was merged. This fixed the arm64
> problem linked in the thread but there seems to be another issue in
> timekeeping.c:
>
> /kernel/time/timekeeping.c:2051: undefined reference to `____ilog2_NaN'
>
> Fedora enables CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE so I think the
> compiler is calculating a possible constant of 0 once again.
>
> Any ideas about a proper fix?

Huh. So if I understand this, its because we don't explicit checks for
offsec or cycle_interval being zero in:

        shift = ilog2(offset) - ilog2(tk->cycle_interval);

Right?

Clearly that case isn't expected to happen, but if it did we'd want
the result of ilog2 to return zero.  So I'm not sure if that
order_base_2() function is maybe the right function to use as it has
an explict zero check?

thanks
-john

[toc] | [next] | [standalone]

#1587959

FromArd Biesheuvel <ard.biesheuvel@linaro.org>
Date2017-02-24 22:50 +0100
Message-ID<tevUt-6cZ-1@gated-at.bofh.it>
In reply to#1587949
On 24 February 2017 at 21:25, John Stultz <john.stultz@linaro.org> wrote:
> On Thu, Feb 23, 2017 at 10:43 AM, Laura Abbott <labbott@redhat.com> wrote:
>> Hi,
>>
>> Fedora was previously carrying a workaround for a gcc7 issue reported
>> on arm64 http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/461597.html.
>> The workaround got rid of __ilog2_NaN. I dropped the patch this morning
>> because a proper fix (29905b52fad0 ("log2: make order_base_2() behave
>> correctly on const input value zero")) was merged. This fixed the arm64
>> problem linked in the thread but there seems to be another issue in
>> timekeeping.c:
>>
>> /kernel/time/timekeeping.c:2051: undefined reference to `____ilog2_NaN'
>>
>> Fedora enables CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE so I think the
>> compiler is calculating a possible constant of 0 once again.
>>
>> Any ideas about a proper fix?
>
> Huh. So if I understand this, its because we don't explicit checks for
> offsec or cycle_interval being zero in:
>
>         shift = ilog2(offset) - ilog2(tk->cycle_interval);
>
> Right?
>
> Clearly that case isn't expected to happen, but if it did we'd want
> the result of ilog2 to return zero.  So I'm not sure if that
> order_base_2() function is maybe the right function to use as it has
> an explict zero check?
>

The problem is really that GCC splits off a constant folded clone
where one of these variables is a constant 0. In the order_base_2()
case, we could sidestep it by fixing an existing issue with the
function itself, but in this case, it is ilog2() itself that is
affected.

Laura, does the below make any difference at all?

diff --git a/include/linux/log2.h b/include/linux/log2.h
index fd7ff3d91e6a..cf4e5bb662bd 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -85,7 +85,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
 #define ilog2(n)                               \
 (                                              \
        __builtin_constant_p(n) ? (             \
-               (n) < 1 ? ____ilog2_NaN() :     \
+               __builtin_expect((n) < 1, 0) ?  \
+                       ____ilog2_NaN() :       \
                (n) & (1ULL << 63) ? 63 :       \
                (n) & (1ULL << 62) ? 62 :       \
                (n) & (1ULL << 61) ? 61 :       \

[toc] | [prev] | [next] | [standalone]

#1588004

FromLaura Abbott <labbott@redhat.com>
Date2017-02-25 00:40 +0100
Message-ID<texCV-7wi-5@gated-at.bofh.it>
In reply to#1587959
On 02/24/2017 01:45 PM, Ard Biesheuvel wrote:
> On 24 February 2017 at 21:25, John Stultz <john.stultz@linaro.org> wrote:
>> On Thu, Feb 23, 2017 at 10:43 AM, Laura Abbott <labbott@redhat.com> wrote:
>>> Hi,
>>>
>>> Fedora was previously carrying a workaround for a gcc7 issue reported
>>> on arm64 http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/461597.html.
>>> The workaround got rid of __ilog2_NaN. I dropped the patch this morning
>>> because a proper fix (29905b52fad0 ("log2: make order_base_2() behave
>>> correctly on const input value zero")) was merged. This fixed the arm64
>>> problem linked in the thread but there seems to be another issue in
>>> timekeeping.c:
>>>
>>> /kernel/time/timekeeping.c:2051: undefined reference to `____ilog2_NaN'
>>>
>>> Fedora enables CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE so I think the
>>> compiler is calculating a possible constant of 0 once again.
>>>
>>> Any ideas about a proper fix?
>>
>> Huh. So if I understand this, its because we don't explicit checks for
>> offsec or cycle_interval being zero in:
>>
>>         shift = ilog2(offset) - ilog2(tk->cycle_interval);
>>
>> Right?
>>
>> Clearly that case isn't expected to happen, but if it did we'd want
>> the result of ilog2 to return zero.  So I'm not sure if that
>> order_base_2() function is maybe the right function to use as it has
>> an explict zero check?
>>
> 
> The problem is really that GCC splits off a constant folded clone
> where one of these variables is a constant 0. In the order_base_2()
> case, we could sidestep it by fixing an existing issue with the
> function itself, but in this case, it is ilog2() itself that is
> affected.
> 
> Laura, does the below make any difference at all?
> 
> diff --git a/include/linux/log2.h b/include/linux/log2.h
> index fd7ff3d91e6a..cf4e5bb662bd 100644
> --- a/include/linux/log2.h
> +++ b/include/linux/log2.h
> @@ -85,7 +85,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
>  #define ilog2(n)                               \
>  (                                              \
>         __builtin_constant_p(n) ? (             \
> -               (n) < 1 ? ____ilog2_NaN() :     \
> +               __builtin_expect((n) < 1, 0) ?  \
> +                       ____ilog2_NaN() :       \
>                 (n) & (1ULL << 63) ? 63 :       \
>                 (n) & (1ULL << 62) ? 62 :       \
>                 (n) & (1ULL << 61) ? 61 :       \
> 

No, still see the same issue.

Thanks,
Laura

[toc] | [prev] | [next] | [standalone]

#1588086

FromMarkus Trippelsdorf <markus@trippelsdorf.de>
Date2017-02-25 09:20 +0100
Message-ID<teFKa-4VJ-5@gated-at.bofh.it>
In reply to#1588004
On 2017.02.24 at 15:33 -0800, Laura Abbott wrote:
> On 02/24/2017 01:45 PM, Ard Biesheuvel wrote:
> > On 24 February 2017 at 21:25, John Stultz <john.stultz@linaro.org> wrote:
> >> On Thu, Feb 23, 2017 at 10:43 AM, Laura Abbott <labbott@redhat.com> wrote:
> >>> Hi,
> >>>
> >>> Fedora was previously carrying a workaround for a gcc7 issue reported
> >>> on arm64 http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/461597.html.
> >>> The workaround got rid of __ilog2_NaN. I dropped the patch this morning
> >>> because a proper fix (29905b52fad0 ("log2: make order_base_2() behave
> >>> correctly on const input value zero")) was merged. This fixed the arm64
> >>> problem linked in the thread but there seems to be another issue in
> >>> timekeeping.c:
> >>>
> >>> /kernel/time/timekeeping.c:2051: undefined reference to `____ilog2_NaN'
> >>>
> >>> Fedora enables CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE so I think the
> >>> compiler is calculating a possible constant of 0 once again.
> >>>
> >>> Any ideas about a proper fix?
> >>
> >> Huh. So if I understand this, its because we don't explicit checks for
> >> offsec or cycle_interval being zero in:
> >>
> >>         shift = ilog2(offset) - ilog2(tk->cycle_interval);
> >>
> >> Right?
> >>
> >> Clearly that case isn't expected to happen, but if it did we'd want
> >> the result of ilog2 to return zero.  So I'm not sure if that
> >> order_base_2() function is maybe the right function to use as it has
> >> an explict zero check?
> >>
> > 
> > The problem is really that GCC splits off a constant folded clone
> > where one of these variables is a constant 0. In the order_base_2()
> > case, we could sidestep it by fixing an existing issue with the
> > function itself, but in this case, it is ilog2() itself that is
> > affected.
> > 
> > Laura, does the below make any difference at all?
> > 
> > diff --git a/include/linux/log2.h b/include/linux/log2.h
> > index fd7ff3d91e6a..cf4e5bb662bd 100644
> > --- a/include/linux/log2.h
> > +++ b/include/linux/log2.h
> > @@ -85,7 +85,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
> >  #define ilog2(n)                               \
> >  (                                              \
> >         __builtin_constant_p(n) ? (             \
> > -               (n) < 1 ? ____ilog2_NaN() :     \
> > +               __builtin_expect((n) < 1, 0) ?  \
> > +                       ____ilog2_NaN() :       \
> >                 (n) & (1ULL << 63) ? 63 :       \
> >                 (n) & (1ULL << 62) ? 62 :       \
> >                 (n) & (1ULL << 61) ? 61 :       \
> > 
> 
> No, still see the same issue.

Why not simply get rid of the ____ilog2_NaN thing altogether?

diff --git a/include/linux/log2.h b/include/linux/log2.h
index ef3d4f67118c..07ef24eedf83 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -16,12 +16,6 @@
 #include <linux/bitops.h>
 
 /*
- * deal with unrepresentable constant logarithms
- */
-extern __attribute__((const, noreturn))
-int ____ilog2_NaN(void);
-
-/*
  * non-constant log of base 2 calculators
  * - the arch may override these in asm/bitops.h if they can be implemented
  *   more efficiently than using fls() and fls64()
@@ -85,7 +79,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
 #define ilog2(n)				\
 (						\
 	__builtin_constant_p(n) ? (		\
-		(n) < 1 ? ____ilog2_NaN() :	\
+		(n) < 1 ? 0 :			\
 		(n) & (1ULL << 63) ? 63 :	\
 		(n) & (1ULL << 62) ? 62 :	\
 		(n) & (1ULL << 61) ? 61 :	\
@@ -149,9 +143,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
 		(n) & (1ULL <<  3) ?  3 :	\
 		(n) & (1ULL <<  2) ?  2 :	\
 		(n) & (1ULL <<  1) ?  1 :	\
-		(n) & (1ULL <<  0) ?  0 :	\
-		____ilog2_NaN()			\
-				   ) :		\
+		0		   ) :		\
 	(sizeof(n) <= 4) ?			\
 	__ilog2_u32(n) :			\
 	__ilog2_u64(n)				\

-- 
Markus

[toc] | [prev] | [next] | [standalone]

#1588097

FromArd Biesheuvel <ard.biesheuvel@linaro.org>
Date2017-02-25 10:20 +0100
Message-ID<teGGd-5Fx-1@gated-at.bofh.it>
In reply to#1588086
On 25 February 2017 at 08:18, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2017.02.24 at 15:33 -0800, Laura Abbott wrote:
>> On 02/24/2017 01:45 PM, Ard Biesheuvel wrote:
>> > On 24 February 2017 at 21:25, John Stultz <john.stultz@linaro.org> wrote:
>> >> On Thu, Feb 23, 2017 at 10:43 AM, Laura Abbott <labbott@redhat.com> wrote:
>> >>> Hi,
>> >>>
>> >>> Fedora was previously carrying a workaround for a gcc7 issue reported
>> >>> on arm64 http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/461597.html.
>> >>> The workaround got rid of __ilog2_NaN. I dropped the patch this morning
>> >>> because a proper fix (29905b52fad0 ("log2: make order_base_2() behave
>> >>> correctly on const input value zero")) was merged. This fixed the arm64
>> >>> problem linked in the thread but there seems to be another issue in
>> >>> timekeeping.c:
>> >>>
>> >>> /kernel/time/timekeeping.c:2051: undefined reference to `____ilog2_NaN'
>> >>>
>> >>> Fedora enables CONFIG_CLOCKSOURCE_VALIDATE_LAST_CYCLE so I think the
>> >>> compiler is calculating a possible constant of 0 once again.
>> >>>
>> >>> Any ideas about a proper fix?
>> >>
>> >> Huh. So if I understand this, its because we don't explicit checks for
>> >> offsec or cycle_interval being zero in:
>> >>
>> >>         shift = ilog2(offset) - ilog2(tk->cycle_interval);
>> >>
>> >> Right?
>> >>
>> >> Clearly that case isn't expected to happen, but if it did we'd want
>> >> the result of ilog2 to return zero.  So I'm not sure if that
>> >> order_base_2() function is maybe the right function to use as it has
>> >> an explict zero check?
>> >>
>> >
>> > The problem is really that GCC splits off a constant folded clone
>> > where one of these variables is a constant 0. In the order_base_2()
>> > case, we could sidestep it by fixing an existing issue with the
>> > function itself, but in this case, it is ilog2() itself that is
>> > affected.
>> >
>> > Laura, does the below make any difference at all?
>> >
>> > diff --git a/include/linux/log2.h b/include/linux/log2.h
>> > index fd7ff3d91e6a..cf4e5bb662bd 100644
>> > --- a/include/linux/log2.h
>> > +++ b/include/linux/log2.h
>> > @@ -85,7 +85,8 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
>> >  #define ilog2(n)                               \
>> >  (                                              \
>> >         __builtin_constant_p(n) ? (             \
>> > -               (n) < 1 ? ____ilog2_NaN() :     \
>> > +               __builtin_expect((n) < 1, 0) ?  \
>> > +                       ____ilog2_NaN() :       \
>> >                 (n) & (1ULL << 63) ? 63 :       \
>> >                 (n) & (1ULL << 62) ? 62 :       \
>> >                 (n) & (1ULL << 61) ? 61 :       \
>> >
>>
>> No, still see the same issue.
>
> 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.

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.

-- 
Ard.


> diff --git a/include/linux/log2.h b/include/linux/log2.h
> index ef3d4f67118c..07ef24eedf83 100644
> --- a/include/linux/log2.h
> +++ b/include/linux/log2.h
> @@ -16,12 +16,6 @@
>  #include <linux/bitops.h>
>
>  /*
> - * deal with unrepresentable constant logarithms
> - */
> -extern __attribute__((const, noreturn))
> -int ____ilog2_NaN(void);
> -
> -/*
>   * non-constant log of base 2 calculators
>   * - the arch may override these in asm/bitops.h if they can be implemented
>   *   more efficiently than using fls() and fls64()
> @@ -85,7 +79,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
>  #define ilog2(n)                               \
>  (                                              \
>         __builtin_constant_p(n) ? (             \
> -               (n) < 1 ? ____ilog2_NaN() :     \
> +               (n) < 1 ? 0 :                   \
>                 (n) & (1ULL << 63) ? 63 :       \
>                 (n) & (1ULL << 62) ? 62 :       \
>                 (n) & (1ULL << 61) ? 61 :       \
> @@ -149,9 +143,7 @@ unsigned long __rounddown_pow_of_two(unsigned long n)
>                 (n) & (1ULL <<  3) ?  3 :       \
>                 (n) & (1ULL <<  2) ?  2 :       \
>                 (n) & (1ULL <<  1) ?  1 :       \
> -               (n) & (1ULL <<  0) ?  0 :       \
> -               ____ilog2_NaN()                 \
> -                                  ) :          \
> +               0                  ) :          \
>         (sizeof(n) <= 4) ?                      \
>         __ilog2_u32(n) :                        \
>         __ilog2_u64(n)                          \
>
> --
> Markus

[toc] | [prev] | [next] | [standalone]

#1588125

FromMarkus Trippelsdorf <markus@trippelsdorf.de>
Date2017-02-25 12:20 +0100
Message-ID<teIyl-6Ye-1@gated-at.bofh.it>
In reply to#1588097
On 2017.02.25 at 09:11 +0000, Ard Biesheuvel wrote:
> On 25 February 2017 at 08:18, Markus Trippelsdorf <markus@trippelsdorf.de> 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.)

-- 
Markus

[toc] | [prev] | [next] | [standalone]

#1588131

FromArd Biesheuvel <ard.biesheuvel@linaro.org>
Date2017-02-25 12:40 +0100
Message-ID<teIRI-74O-11@gated-at.bofh.it>
In reply to#1588125
On 25 February 2017 at 11:09, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2017.02.25 at 09:11 +0000, Ard Biesheuvel wrote:
>> On 25 February 2017 at 08:18, Markus Trippelsdorf <markus@trippelsdorf.de> 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()?

[toc] | [prev] | [next] | [standalone]

#1588133

FromArd Biesheuvel <ard.biesheuvel@linaro.org>
Date2017-02-25 13:00 +0100
Message-ID<teJb3-7dx-1@gated-at.bofh.it>
In reply to#1588131

> On 25 Feb 2017, at 11:23, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> On 25 February 2017 at 11:09, Markus Trippelsdorf
> <markus@trippelsdorf.de> wrote:
>> On 2017.02.25 at 09:11 +0000, Ard Biesheuvel wrote:
>>>> On 25 February 2017 at 08:18, Markus Trippelsdorf <markus@trippelsdorf.de> 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.

[toc] | [prev] | [next] | [standalone]

#1589924

FromLaura Abbott <labbott@redhat.com>
Date2017-03-01 02:20 +0100
Message-ID<tg15T-3SJ-11@gated-at.bofh.it>
In reply to#1588133
On 02/25/2017 03:50 AM, Ard Biesheuvel wrote:
> 
> 
>> On 25 Feb 2017, at 11:23, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>> On 25 February 2017 at 11:09, Markus Trippelsdorf
>> <markus@trippelsdorf.de> wrote:
>>> On 2017.02.25 at 09:11 +0000, Ard Biesheuvel wrote:
>>>>> On 25 February 2017 at 08:18, Markus Trippelsdorf <markus@trippelsdorf.de> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web