Messages in this thread | | | Subject | Re: Build failure with v4.9-rc1 and GCC trunk -- compiler weirdness | From | Laura Abbott <> | Date | Thu, 2 Feb 2017 07:43:39 -0800 |
| |
On 02/02/2017 01:17 AM, Ard Biesheuvel wrote: > On 1 February 2017 at 21:50, Laura Abbott <labbott@redhat.com> wrote: >> On 02/01/2017 09:36 AM, Ard Biesheuvel wrote: >>> On 1 February 2017 at 16:58, Laura Abbott <labbott@redhat.com> wrote: >>>> On 10/19/2016 09:22 AM, Will Deacon wrote: >>>>> On Wed, Oct 19, 2016 at 09:01:33AM -0700, Linus Torvalds wrote: >>>>>> On Wed, Oct 19, 2016 at 8:56 AM, Markus Trippelsdorf >>>>>> <markus@trippelsdorf.de> wrote: >>>>>>> On 2016.10.19 at 08:55 -0700, Linus Torvalds wrote: >>>>>>>> >>>>>>>> Well, in the meantime we apparently have to live with it. Unless Will >>>>>>>> is using some unreleased gcc version that nobody else is using and we >>>>>>>> can just ignore it? >>>>>>> >>>>>>> Yes, he is using gcc-7 that is unreleased. (It will be released April >>>>>>> next year.) >>>>>> >>>>>> Ahh, self-built? So it's not part of some experimental ARM distro >>>>>> setup and this will be annoying lots of people? >>>>> >>>>> Our friendly compiler guys built it, but it's just a snapshot of trunk, >>>>> so it's all heading towards GCC 7.0. AFAIU, the problematic optimisation >>>>> is also a mid-end pass, so it would affect other architectures too. >>>>> >>>>>> If so, still think that we could just get rid of the ____ilog2_NaN() >>>>>> thing as it's not _that_ important, but it's certainly not very >>>>>> high-priority. Will can do it in his tree too for testing, and it can >>>>>> remind people to get the gcc problem fixed. >>>>> >>>>> I'm carrying the diff below, which fixes arm64 defconfig, but I'm worried >>>>> that we might be relying on this trick elsewhere. The arm __bad_cmpxchg >>>>> function, for example. >>>>> >>>>> Will >>>>> >>>>> --->8 >>>>> >>>>> diff --git a/include/linux/log2.h b/include/linux/log2.h >>>>> index fd7ff3d91e6a..9cf5ad69065d 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) \ >>>>> @@ -194,7 +186,6 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>>>> * @n: parameter >>>>> * >>>>> * The first few values calculated by this routine: >>>>> - * ob2(0) = 0 >>>>> * ob2(1) = 0 >>>>> * ob2(2) = 1 >>>>> * ob2(3) = 2 >>>>> >>>> >>>> Reviving this thread as gcc 7 has now hit Fedora rawhide and has this >>>> same issue. I pulled in the above patch from Will as a temporary work >>>> around for building. It didn't look like there was consensus on a >>>> permanent solution though from the thread. >>>> >>> >>> I still think order_base_2() is broken, since it may invoke >>> roundup_pow_of_two() with an input value that is documented as >>> producing undefined output. I would argue that the below is the >>> correct fix. >>> >>> diff --git a/include/linux/log2.h b/include/linux/log2.h >>> index fd7ff3d91e6a..46523731bec0 100644 >>> --- a/include/linux/log2.h >>> +++ b/include/linux/log2.h >>> @@ -203,6 +203,18 @@ unsigned long __rounddown_pow_of_two(unsigned long n) >>> * ... and so on. >>> */ >>> >>> -#define order_base_2(n) ilog2(roundup_pow_of_two(n)) >>> +static inline __attribute__((__const__)) >>> +unsigned long __order_base_2(unsigned long n) >>> +{ >>> + return n ? 1UL << fls_long(n - 1) : 1; >>> +} >>> + >>> +#define order_base_2(n) \ >>> +( \ >>> + __builtin_constant_p(n) ? ( \ >>> + ((n) < 2) ? (n) : \ >>> + ilog2((n) - 1) + 1) : \ >>> + ilog2(__order_base_2(n)) \ >>> + ) >>> >>> #endif /* _LINUX_LOG2_H */ >>> >> >> This fixes the problem although the comments should be updated >> as well. > > This brings order_base_2() in line with the comments, so I am not sure > what you'd want to update here? >
ob2(1) = 1 for the __builtin_constant_p case which doesn't match the comment of ob2(1) = 0. So my statement should actually be is this correct?
Thanks, Laura
| |