Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()

Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 2572 bytes --]

Hi Arnd,

On Tue, Dec 19, 2017 at 12:39:33PM +0100, Arnd Bergmann wrote:
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index 5d595cfdb2c4..66cfdad68f7e 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -205,6 +205,15 @@
>  #endif
>  
>  /*
> + * calling noreturn functions, __builtin_unreachable() and __builtin_trap()
> + * confuse the stack allocation in gcc, leading to overly large stack
> + * frames, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
> + *
> + * Adding an empty inline assembly before it works around the problem
> + */
> +#define barrier_before_unreachable() asm volatile("")
> +
> +/*
>   * Mark a position in code as unreachable.  This can be used to
>   * suppress control flow warnings after asm blocks that transfer
>   * control elsewhere.
> @@ -214,7 +223,11 @@
>   * unreleased.  Really, we need to have autoconf for the kernel.
>   */
>  #define unreachable() \
> -	do { annotate_unreachable(); __builtin_unreachable(); } while (0)
> +	do {					\
> +		annotate_unreachable();		\
> +		barrier_before_unreachable();	\
> +		__builtin_unreachable();	\
> +	} while (0)

Unfortunately this breaks microMIPS builds (e.g. MIPS
micro32r2_defconfig and micro32r2el_defconfig) on gcc 7.2, due to the
lack of .insn in the asm volatile. Because of the
__builtin_unreachable() there is no code following it. Without the empty
asm the compiler will apparently put the .insn there automatically, but
with the empty asm it doesn't. Therefore the assembler won't treat an
immediately preceeding label as pointing at 16-bit microMIPS
instructions which need the ISA bit set, i.e. bit 0 of the address.
This causes assembler errors since the branch target is treated as a
different ISA mode:

arch/mips/mm/dma-default.s:3265: Error: branch to a symbol in another ISA mode
arch/mips/mm/dma-default.s:5027: Error: branch to a symbol in another ISA mode

Due to a compiler bug on gcc 4.9.2 -> somewhere before 7.2, Paul
submitted these patches a while back:
https://patchwork.linux-mips.org/patch/13360/
https://patchwork.linux-mips.org/patch/13361/

Your patch (suitably fixed for microMIPS) would I imagine fix that issue
too (it certainly fixes the resulting link error on microMIPS builds
with an old toolchain).

Before I forward port those patches to add .insn for MIPS, is that sort
of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
override barrier_before_unreachable()) an acceptable fix?

Thanks
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()

[Arnd Bergmann]

On Wed, Apr 11, 2018 at 12:48 AM, James Hogan <jhogan@kernel.org> wrote:
> Hi Arnd,
>
> On Tue, Dec 19, 2017 at 12:39:33PM +0100, Arnd Bergmann wrote:
>> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
>> index 5d595cfdb2c4..66cfdad68f7e 100644
>> --- a/include/linux/compiler-gcc.h
>> +++ b/include/linux/compiler-gcc.h
>> @@ -205,6 +205,15 @@
>>  #endif
>>
>>  /*
>> + * calling noreturn functions, __builtin_unreachable() and __builtin_trap()
>> + * confuse the stack allocation in gcc, leading to overly large stack
>> + * frames, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
>> + *
>> + * Adding an empty inline assembly before it works around the problem
>> + */
>> +#define barrier_before_unreachable() asm volatile("")
>> +
>> +/*
>>   * Mark a position in code as unreachable.  This can be used to
>>   * suppress control flow warnings after asm blocks that transfer
>>   * control elsewhere.
>> @@ -214,7 +223,11 @@
>>   * unreleased.  Really, we need to have autoconf for the kernel.
>>   */
>>  #define unreachable() \
>> -     do { annotate_unreachable(); __builtin_unreachable(); } while (0)
>> +     do {                                    \
>> +             annotate_unreachable();         \
>> +             barrier_before_unreachable();   \
>> +             __builtin_unreachable();        \
>> +     } while (0)
>
> Unfortunately this breaks microMIPS builds (e.g. MIPS
> micro32r2_defconfig and micro32r2el_defconfig) on gcc 7.2, due to the
> lack of .insn in the asm volatile. Because of the
> __builtin_unreachable() there is no code following it. Without the empty
> asm the compiler will apparently put the .insn there automatically, but
> with the empty asm it doesn't. Therefore the assembler won't treat an
> immediately preceeding label as pointing at 16-bit microMIPS
> instructions which need the ISA bit set, i.e. bit 0 of the address.
> This causes assembler errors since the branch target is treated as a
> different ISA mode:
>
> arch/mips/mm/dma-default.s:3265: Error: branch to a symbol in another ISA mode
> arch/mips/mm/dma-default.s:5027: Error: branch to a symbol in another ISA mode

Ok, I see.

> Due to a compiler bug on gcc 4.9.2 -> somewhere before 7.2, Paul
> submitted these patches a while back:
> https://patchwork.linux-mips.org/patch/13360/
> https://patchwork.linux-mips.org/patch/13361/
>
> Your patch (suitably fixed for microMIPS) would I imagine fix that issue
> too (it certainly fixes the resulting link error on microMIPS builds
> with an old toolchain).
>
> Before I forward port those patches to add .insn for MIPS, is that sort
> of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
> override barrier_before_unreachable()) an acceptable fix?

That sounds fine to me. However, I would suggest making that
asm/compiler.h instead of asm/compiler-gcc.h, so we can also
use the same file to include workarounds for clang if needed.

       Arnd

Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()

[Arnd Bergmann]

On Wed, Apr 11, 2018 at 12:48 AM, James Hogan <jhogan@kernel.org> wrote:
> Hi Arnd,
>
> On Tue, Dec 19, 2017 at 12:39:33PM +0100, Arnd Bergmann wrote:
>> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
>> index 5d595cfdb2c4..66cfdad68f7e 100644
>> --- a/include/linux/compiler-gcc.h
>> +++ b/include/linux/compiler-gcc.h
>> @@ -205,6 +205,15 @@
>>  #endif
>>
>>  /*
>> + * calling noreturn functions, __builtin_unreachable() and __builtin_trap()
>> + * confuse the stack allocation in gcc, leading to overly large stack
>> + * frames, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
>> + *
>> + * Adding an empty inline assembly before it works around the problem
>> + */
>> +#define barrier_before_unreachable() asm volatile("")
>> +
>> +/*
>>   * Mark a position in code as unreachable.  This can be used to
>>   * suppress control flow warnings after asm blocks that transfer
>>   * control elsewhere.
>> @@ -214,7 +223,11 @@
>>   * unreleased.  Really, we need to have autoconf for the kernel.
>>   */
>>  #define unreachable() \
>> -     do { annotate_unreachable(); __builtin_unreachable(); } while (0)
>> +     do {                                    \
>> +             annotate_unreachable();         \
>> +             barrier_before_unreachable();   \
>> +             __builtin_unreachable();        \
>> +     } while (0)
>
> Unfortunately this breaks microMIPS builds (e.g. MIPS
> micro32r2_defconfig and micro32r2el_defconfig) on gcc 7.2, due to the
> lack of .insn in the asm volatile. Because of the
> __builtin_unreachable() there is no code following it. Without the empty
> asm the compiler will apparently put the .insn there automatically, but
> with the empty asm it doesn't. Therefore the assembler won't treat an
> immediately preceeding label as pointing at 16-bit microMIPS
> instructions which need the ISA bit set, i.e. bit 0 of the address.
> This causes assembler errors since the branch target is treated as a
> different ISA mode:
>
> arch/mips/mm/dma-default.s:3265: Error: branch to a symbol in another ISA mode
> arch/mips/mm/dma-default.s:5027: Error: branch to a symbol in another ISA mode

Ok, I see.

> Due to a compiler bug on gcc 4.9.2 -> somewhere before 7.2, Paul
> submitted these patches a while back:
> https://patchwork.linux-mips.org/patch/13360/
> https://patchwork.linux-mips.org/patch/13361/
>
> Your patch (suitably fixed for microMIPS) would I imagine fix that issue
> too (it certainly fixes the resulting link error on microMIPS builds
> with an old toolchain).
>
> Before I forward port those patches to add .insn for MIPS, is that sort
> of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
> override barrier_before_unreachable()) an acceptable fix?

That sounds fine to me. However, I would suggest making that
asm/compiler.h instead of asm/compiler-gcc.h, so we can also
use the same file to include workarounds for clang if needed.

       Arnd

Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Wed, Apr 11, 2018 at 09:30:56AM +0200, Arnd Bergmann wrote:
> On Wed, Apr 11, 2018 at 12:48 AM, James Hogan <jhogan@kernel.org> wrote:
> > Before I forward port those patches to add .insn for MIPS, is that sort
> > of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
> > override barrier_before_unreachable()) an acceptable fix?
> 
> That sounds fine to me. However, I would suggest making that
> asm/compiler.h instead of asm/compiler-gcc.h, so we can also
> use the same file to include workarounds for clang if needed.

Yes, though there are a few asm/compiler.h's already, and the alpha one
includes linux/compiler.h before undefining inline, so seems to have its
own specific purpose...

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Wed, Apr 11, 2018 at 09:30:56AM +0200, Arnd Bergmann wrote:
> On Wed, Apr 11, 2018 at 12:48 AM, James Hogan <jhogan@kernel.org> wrote:
> > Before I forward port those patches to add .insn for MIPS, is that sort
> > of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
> > override barrier_before_unreachable()) an acceptable fix?
> 
> That sounds fine to me. However, I would suggest making that
> asm/compiler.h instead of asm/compiler-gcc.h, so we can also
> use the same file to include workarounds for clang if needed.

Yes, though there are a few asm/compiler.h's already, and the alpha one
includes linux/compiler.h before undefining inline, so seems to have its
own specific purpose...

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()

[Arnd Bergmann]

On Wed, Apr 11, 2018 at 11:54 AM, James Hogan <jhogan@kernel.org> wrote:
> On Wed, Apr 11, 2018 at 09:30:56AM +0200, Arnd Bergmann wrote:
>> On Wed, Apr 11, 2018 at 12:48 AM, James Hogan <jhogan@kernel.org> wrote:
>> > Before I forward port those patches to add .insn for MIPS, is that sort
>> > of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
>> > override barrier_before_unreachable()) an acceptable fix?
>>
>> That sounds fine to me. However, I would suggest making that
>> asm/compiler.h instead of asm/compiler-gcc.h, so we can also
>> use the same file to include workarounds for clang if needed.
>
> Yes, though there are a few asm/compiler.h's already, and the alpha one
> includes linux/compiler.h before undefining inline, so seems to have its
> own specific purpose...

Interesting. For the other ones, including asm/compiler.h from linux/compiler.h
seems appropriate though, so the question would be what to do with the
alpha case. I think we can simply remove that header file and replace
it with this patch:

diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
index b2022885ced8..5502404f54cd 100644
--- a/arch/alpha/Kconfig
+++ b/arch/alpha/Kconfig
@@ -81,6 +81,9 @@ config PGTABLE_LEVELS
        int
        default 3

+config OPTIMIZE_INLINING
+       def_bool y
+
 source "init/Kconfig"
 source "kernel/Kconfig.freezer"

which should have the same effect.

Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()

[Arnd Bergmann]

On Wed, Apr 11, 2018 at 11:54 AM, James Hogan <jhogan@kernel.org> wrote:
> On Wed, Apr 11, 2018 at 09:30:56AM +0200, Arnd Bergmann wrote:
>> On Wed, Apr 11, 2018 at 12:48 AM, James Hogan <jhogan@kernel.org> wrote:
>> > Before I forward port those patches to add .insn for MIPS, is that sort
>> > of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
>> > override barrier_before_unreachable()) an acceptable fix?
>>
>> That sounds fine to me. However, I would suggest making that
>> asm/compiler.h instead of asm/compiler-gcc.h, so we can also
>> use the same file to include workarounds for clang if needed.
>
> Yes, though there are a few asm/compiler.h's already, and the alpha one
> includes linux/compiler.h before undefining inline, so seems to have its
> own specific purpose...

Interesting. For the other ones, including asm/compiler.h from linux/compiler.h
seems appropriate though, so the question would be what to do with the
alpha case. I think we can simply remove that header file and replace
it with this patch:

diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
index b2022885ced8..5502404f54cd 100644
--- a/arch/alpha/Kconfig
+++ b/arch/alpha/Kconfig
@@ -81,6 +81,9 @@ config PGTABLE_LEVELS
        int
        default 3

+config OPTIMIZE_INLINING
+       def_bool y
+
 source "init/Kconfig"
 source "kernel/Kconfig.freezer"

which should have the same effect.

Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1682 bytes --]

On Wed, Apr 11, 2018 at 12:08:51PM +0200, Arnd Bergmann wrote:
> On Wed, Apr 11, 2018 at 11:54 AM, James Hogan <jhogan@kernel.org> wrote:
> > On Wed, Apr 11, 2018 at 09:30:56AM +0200, Arnd Bergmann wrote:
> >> On Wed, Apr 11, 2018 at 12:48 AM, James Hogan <jhogan@kernel.org> wrote:
> >> > Before I forward port those patches to add .insn for MIPS, is that sort
> >> > of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
> >> > override barrier_before_unreachable()) an acceptable fix?
> >>
> >> That sounds fine to me. However, I would suggest making that
> >> asm/compiler.h instead of asm/compiler-gcc.h, so we can also
> >> use the same file to include workarounds for clang if needed.
> >
> > Yes, though there are a few asm/compiler.h's already, and the alpha one
> > includes linux/compiler.h before undefining inline, so seems to have its
> > own specific purpose...
> 
> Interesting. For the other ones, including asm/compiler.h from linux/compiler.h
> seems appropriate though, so the question would be what to do with the
> alpha case. I think we can simply remove that header file and replace
> it with this patch:
> 
> diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
> index b2022885ced8..5502404f54cd 100644
> --- a/arch/alpha/Kconfig
> +++ b/arch/alpha/Kconfig
> @@ -81,6 +81,9 @@ config PGTABLE_LEVELS
>         int
>         default 3
> 
> +config OPTIMIZE_INLINING
> +       def_bool y
> +
>  source "init/Kconfig"
>  source "kernel/Kconfig.freezer"
> 
> which should have the same effect.

Hmm yes, and I suppose alpha would need ARCH_SUPPORTS_OPTIMIZED_INLINING
too. I'll give it a try.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1682 bytes --]

On Wed, Apr 11, 2018 at 12:08:51PM +0200, Arnd Bergmann wrote:
> On Wed, Apr 11, 2018 at 11:54 AM, James Hogan <jhogan@kernel.org> wrote:
> > On Wed, Apr 11, 2018 at 09:30:56AM +0200, Arnd Bergmann wrote:
> >> On Wed, Apr 11, 2018 at 12:48 AM, James Hogan <jhogan@kernel.org> wrote:
> >> > Before I forward port those patches to add .insn for MIPS, is that sort
> >> > of approach (an arch specific asm/compiler-gcc.h to allow MIPS to
> >> > override barrier_before_unreachable()) an acceptable fix?
> >>
> >> That sounds fine to me. However, I would suggest making that
> >> asm/compiler.h instead of asm/compiler-gcc.h, so we can also
> >> use the same file to include workarounds for clang if needed.
> >
> > Yes, though there are a few asm/compiler.h's already, and the alpha one
> > includes linux/compiler.h before undefining inline, so seems to have its
> > own specific purpose...
> 
> Interesting. For the other ones, including asm/compiler.h from linux/compiler.h
> seems appropriate though, so the question would be what to do with the
> alpha case. I think we can simply remove that header file and replace
> it with this patch:
> 
> diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
> index b2022885ced8..5502404f54cd 100644
> --- a/arch/alpha/Kconfig
> +++ b/arch/alpha/Kconfig
> @@ -81,6 +81,9 @@ config PGTABLE_LEVELS
>         int
>         default 3
> 
> +config OPTIMIZE_INLINING
> +       def_bool y
> +
>  source "init/Kconfig"
>  source "kernel/Kconfig.freezer"
> 
> which should have the same effect.

Hmm yes, and I suppose alpha would need ARCH_SUPPORTS_OPTIMIZED_INLINING
too. I'll give it a try.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]