[PATCH] Default setting of the ARM_UNWIND option

[PATCH] Default setting of the ARM_UNWIND option

[Claudio Scordino]

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

[email cut-and-pasted from a private discussion...]
>>    moving from release 2.6.27 to release 2.6.32-rc4, my ARM board 
>> (derived from AT91SAM9623-EK) started hanging at the initial 
>> "Calibrating delay loop" message.
>>
>> After some inspection, I found out the problem to be with commit 
>> adf8b37bafc1495393201a2ae4235846371870d0.
>>
>> In practice, it enables stack unwinding support by default. However, if 
>> enabled, it hangs my machine.
>>
>> The problem might be with my compiler (according to the help, stack 
>> unwinding only works with EABI compilers).
>>
>> I'm using a CodeSourcery gcc 4.2.1 "arm-none-linux-gnueabi".
>>     
>
> I think 4.2.1 generates really buggy code with -fno-frame-pointer,
> nothing to do with the stack unwinding implementation in the kernel. I
> would recommend you either use a newer compiler or disables ARM_UNWIND
> in your .config (which automatically re-enables FRAMEPOINTER).
>   
Hi Catalin,

   thank you for your answer.

In my opinion, we should change the default setting of this option.

Having "yes" as default policy for ARM_UNWIND means that, by default, 
the kernel does not boot if a user has a buggy compiler (like me) and 
he/she doesn't explicitly disable the ARM_UNWIND option.

Consider that people may not know that their problem is related to the 
ARM_UNWIND option. Therefore, they may waste time just bisecting to 
understand where their problem really is.

Therefore, my suggestion is to change the default policy of this option 
as shown in the attached patch, to avoid a potential waste of time of 
developers. Users who know that they can enable this option, will enable 
it.

Let me know what you think about this suggestion.

Many thanks and my best regards,

               Claudio



[-- Attachment #2: 0001-Disable-stack-unwinding-support-by-default-since-it.patch --]
[-- Type: text/x-patch, Size: 1171 bytes --]

>From 3b688dbebb5fb3345e48cd491eb387434db165da Mon Sep 17 00:00:00 2001
From: Claudio Scordino <claudio@evidence.eu.com>
Date: Mon, 19 Oct 2009 11:59:16 +0200
Subject: [PATCH 1/1] Disable stack unwinding support by default, since it does not work on some buggy or not-EABI compilers.


Signed-off-by: Claudio Scordino <claudio@evidence.eu.com>
---
 arch/arm/Kconfig.debug |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 1a6f70e..925e3fd 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -19,13 +19,13 @@ config FRAME_POINTER
 config ARM_UNWIND
 	bool "Enable stack unwinding support"
 	depends on AEABI && EXPERIMENTAL
-	default y
+	default n
 	help
 	  This option enables stack unwinding support in the kernel
 	  using the information automatically generated by the
 	  compiler. The resulting kernel image is slightly bigger but
 	  the performance is not affected. Currently, this feature
-	  only works with EABI compilers. If unsure say Y.
+	  only works with EABI compilers. If unsure say N.
 
 config DEBUG_USER
 	bool "Verbose user fault messages"
-- 
1.6.0.4

[PATCH] Default setting of the ARM_UNWIND option

[Claudio Scordino]

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

[email cut-and-pasted from a private discussion...]
>>    moving from release 2.6.27 to release 2.6.32-rc4, my ARM board 
>> (derived from AT91SAM9623-EK) started hanging at the initial 
>> "Calibrating delay loop" message.
>>
>> After some inspection, I found out the problem to be with commit 
>> adf8b37bafc1495393201a2ae4235846371870d0.
>>
>> In practice, it enables stack unwinding support by default. However, if 
>> enabled, it hangs my machine.
>>
>> The problem might be with my compiler (according to the help, stack 
>> unwinding only works with EABI compilers).
>>
>> I'm using a CodeSourcery gcc 4.2.1 "arm-none-linux-gnueabi".
>>     
>
> I think 4.2.1 generates really buggy code with -fno-frame-pointer,
> nothing to do with the stack unwinding implementation in the kernel. I
> would recommend you either use a newer compiler or disables ARM_UNWIND
> in your .config (which automatically re-enables FRAMEPOINTER).
>   
Hi Catalin,

    thank you for your answer.

In my opinion, we should change the default setting of this option.

Having "yes" as default policy for ARM_UNWIND means that, by default,
the kernel does not boot if a user has a buggy compiler (like me) and
he/she doesn't explicitly disable the ARM_UNWIND option.

Consider that people may not know that their problem is related to the
ARM_UNWIND option. Therefore, they may waste time just bisecting to
understand where their problem really is.

Therefore, my suggestion is to change the default policy of this option
as shown in the attached patch, to avoid a potential waste of time of
developers. Users who know that they can enable this option, will enable
it.

Let me know what you think about this suggestion.

Many thanks and my best regards,

                Claudio




[-- Attachment #2: 0001-Disable-stack-unwinding-support-by-default-since-it.patch --]
[-- Type: text/x-patch, Size: 1203 bytes --]

From: Claudio Scordino <claudio@evidence.eu.com>
Date: Mon, 19 Oct 2009 11:59:16 +0200
Subject: [PATCH 1/1] Disable stack unwinding support by default, since it does not work on some buggy or not-EABI compilers.

Disable stack unwinding support by default, since it does not work on some
buggy or not-EABI compilers.

Signed-off-by: Claudio Scordino <claudio@evidence.eu.com>
---
 arch/arm/Kconfig.debug |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 1a6f70e..925e3fd 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -19,13 +19,13 @@ config FRAME_POINTER
 config ARM_UNWIND
 	bool "Enable stack unwinding support"
 	depends on AEABI && EXPERIMENTAL
-	default y
+	default n
 	help
 	  This option enables stack unwinding support in the kernel
 	  using the information automatically generated by the
 	  compiler. The resulting kernel image is slightly bigger but
 	  the performance is not affected. Currently, this feature
-	  only works with EABI compilers. If unsure say Y.
+	  only works with EABI compilers. If unsure say N.
 
 config DEBUG_USER
 	bool "Verbose user fault messages"
-- 
1.6.0.4

[PATCH][RE-SUBMIT] Default setting of the ARM_UNWIND option

[Claudio Scordino]

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

Hi all,

          I didn't get any comment on this patch, so I try to re-submit.

My ARM board hanged at the initial "Calibrating delay loop" message.

After some inspection, I found out the problem to be with commit
adf8b37bafc1495393201a2ae4235846371870d0. This commit introduces stack
unwinding for ARM, and set it enabled by default. However, it seems to
not work with buggy or not-EABI compilers.

My suggestion is to keep the feature (which is fine) but change the
default setting of the option (see the attached patch).

Having this option enabled by default, in fact, means that the kernel
does not boot if the user has a wrong compiler. If so, inspecting the
reason of the hang may require too much time (especially for people not
familiar with this option).

Consider that people may not know that their problem is related to the
ARM_UNWIND option. Therefore, they may waste time bisecting just to
understand where their problem really is.

If we disable this option by default, users who know the real meaning of
the option will still be able of setting it to the proper value.

Any comment ?

Many thanks,

                   Claudio







[-- Attachment #2: 0001-Disable-stack-unwinding-support-by-default-since-it.patch --]
[-- Type: text/x-patch, Size: 1210 bytes --]

From: Claudio Scordino <claudio@evidence.eu.com>
Date: Mon, 19 Oct 2009 11:59:16 +0200
Subject: [PATCH 1/1] Disable stack unwinding support by default, since it does not work on some buggy or not-EABI compilers.

Disable stack unwinding support by default, since it does not work on some
buggy or not-EABI compilers.

Signed-off-by: Claudio Scordino <claudio@evidence.eu.com>
---
 arch/arm/Kconfig.debug |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 1a6f70e..925e3fd 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -19,13 +19,13 @@ config FRAME_POINTER
 config ARM_UNWIND
 	bool "Enable stack unwinding support"
 	depends on AEABI && EXPERIMENTAL
-	default y
+	default n
 	help
 	  This option enables stack unwinding support in the kernel
 	  using the information automatically generated by the
 	  compiler. The resulting kernel image is slightly bigger but
 	  the performance is not affected. Currently, this feature
-	  only works with EABI compilers. If unsure say Y.
+	  only works with EABI compilers. If unsure say N.
 
 config DEBUG_USER
 	bool "Verbose user fault messages"
-- 
1.6.0.4

Re: [PATCH][RE-SUBMIT] Default setting of the ARM_UNWIND option

[Catalin Marinas]

On Mon, 2009-10-26 at 09:27 +0100, Claudio Scordino wrote:
> My ARM board hanged at the initial "Calibrating delay loop" message.
> 
> After some inspection, I found out the problem to be with commit
> adf8b37bafc1495393201a2ae4235846371870d0. This commit introduces stack
> unwinding for ARM, and set it enabled by default. However, it seems to
> not work with buggy or not-EABI compilers.
> 
> My suggestion is to keep the feature (which is fine) but change the
> default setting of the option (see the attached patch).

The option still depends on EXPERIMENTAL, so you get ARM_UNWIND on when
enabling that.

I'm more in favour of a #warning on #error in the unwind.c file based on
the compiler version rather than not having it on by default. The reason
is that people reported performance improvements when compiling the
kernel without frame pointers.

-- 
Catalin

Re: [PATCH][RE-SUBMIT] Default setting of the ARM_UNWIND option

[Claudio Scordino]

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

Catalin Marinas ha scritto:
> On Mon, 2009-10-26 at 09:27 +0100, Claudio Scordino wrote:
>   
>> My ARM board hanged at the initial "Calibrating delay loop" message.
>>
>> After some inspection, I found out the problem to be with commit
>> adf8b37bafc1495393201a2ae4235846371870d0. This commit introduces stack
>> unwinding for ARM, and set it enabled by default. However, it seems to
>> not work with buggy or not-EABI compilers.
>>
>> My suggestion is to keep the feature (which is fine) but change the
>> default setting of the option (see the attached patch).
>>     
>
> The option still depends on EXPERIMENTAL, so you get ARM_UNWIND on when
> enabling that.
>
> I'm more in favour of a #warning on #error in the unwind.c file based on
> the compiler version rather than not having it on by default. The reason
> is that people reported performance improvements when compiling the
> kernel without frame pointers.
>   
This solution is fine too (even if I still think that changing the 
default setting is better).

Please, consider the patch in attachment: is it like you would have it ?

BTW, do we have any list of buggy or not-EABI versions of the gcc compiler ?

Many thanks,

            Claudio


[-- Attachment #2: 0001-Disable-stack-unwinding-support-by-default-since-it.patch --]
[-- Type: text/x-patch, Size: 1209 bytes --]

From: Claudio Scordino <claudio@evidence.eu.com>
Date: Mon, 19 Oct 2009 11:59:16 +0200
Subject: [PATCH 1/1] Disable stack unwinding support by default, since it does not work on some buggy or not-EABI compilers.

Disable stack unwinding support by default, since it does not work on some
buggy or not-EABI compilers.

Signed-off-by: Claudio Scordino <claudio@evidence.eu.com>
---
 arch/arm/Kconfig.debug |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 1a6f70e..925e3fd 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -19,13 +19,13 @@ config FRAME_POINTER
 config ARM_UNWIND
 	bool "Enable stack unwinding support"
 	depends on AEABI && EXPERIMENTAL
-	default y
+	default n
 	help
 	  This option enables stack unwinding support in the kernel
 	  using the information automatically generated by the
 	  compiler. The resulting kernel image is slightly bigger but
 	  the performance is not affected. Currently, this feature
-	  only works with EABI compilers. If unsure say Y.
+	  only works with EABI compilers. If unsure say N.
 
 config DEBUG_USER
 	bool "Verbose user fault messages"
-- 
1.6.0.4

Re: [PATCH][RE-SUBMIT] Default setting of the ARM_UNWIND option

[Claudio Scordino]

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

Catalin Marinas ha scritto:
> On Mon, 2009-10-26 at 09:27 +0100, Claudio Scordino wrote:
>   
>> My ARM board hanged at the initial "Calibrating delay loop" message.
>>
>> After some inspection, I found out the problem to be with commit
>> adf8b37bafc1495393201a2ae4235846371870d0. This commit introduces stack
>> unwinding for ARM, and set it enabled by default. However, it seems to
>> not work with buggy or not-EABI compilers.
>>
>> My suggestion is to keep the feature (which is fine) but change the
>> default setting of the option (see the attached patch).
>>     
>
> The option still depends on EXPERIMENTAL, so you get ARM_UNWIND on when
> enabling that.
>
> I'm more in favour of a #warning on #error in the unwind.c file based on
> the compiler version rather than not having it on by default. The reason
> is that people reported performance improvements when compiling the
> kernel without frame pointers.
>   

[Sorry, I sent the wrong patch... This is the new one.]

This solution is fine too (even if I still think that changing the
default setting is better).

Please, consider the patch in attachment: is it like you would have it ?

BTW, do we have any list of buggy or not-EABI versions of the gcc compiler ?

Many thanks,

             Claudio



[-- Attachment #2: 0001-Some-warnings-when-compiling-ARM-unwind-support-with.patch --]
[-- Type: text/x-patch, Size: 1275 bytes --]

From: Claudio Scordino <claudio@evidence.eu.com>
Date: Mon, 26 Oct 2009 17:13:18 +0100
Subject: [PATCH 1/1] Some warnings when compiling ARM unwind support with a buggy or not-EABI compiler.

ARM unwind support is known to build only with EABI and not-buggy compilers.
Now we check the compiler and raise a #warning in case of wrong compiler.

Signed-off-by: Claudio Scordino <claudio@evidence.eu.com>
---
 arch/arm/kernel/unwind.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 39baf11..47345c1 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -26,6 +26,15 @@
  * http://infocenter.arm.com/help/topic/com.arm.doc.subset.swdev.abi/index.html
  */
 
+#if !defined (__ARM_EABI__)
+#warning Your compiler does not have EABI support.
+#warning    ARM unwind support it is known to compile only on EABI compilers.
+#warning    Change compiler or disable ARM_UNWIND option.
+#elif (__GNUC__ == 4 && __GNUC_MINOR__ == 2)
+#warning Your compiler is too buggy; it is known to not compile ARM unwind support.
+#warning    Change compiler or disable ARM_UNWIND option.
+#endif
+
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/module.h>
-- 
1.6.0.4

Re: [PATCH][RE-SUBMIT] Default setting of the ARM_UNWIND option

[Catalin Marinas]

On Wed, 2009-10-28 at 12:37 +0100, Claudio Scordino wrote:
> Catalin Marinas ha scritto:
> > On Mon, 2009-10-26 at 09:27 +0100, Claudio Scordino wrote:
> >   
> >> My ARM board hanged at the initial "Calibrating delay loop" message.
> >>
> >> After some inspection, I found out the problem to be with commit
> >> adf8b37bafc1495393201a2ae4235846371870d0. This commit introduces stack
> >> unwinding for ARM, and set it enabled by default. However, it seems to
> >> not work with buggy or not-EABI compilers.
> >>
> >> My suggestion is to keep the feature (which is fine) but change the
> >> default setting of the option (see the attached patch).
> >
> > The option still depends on EXPERIMENTAL, so you get ARM_UNWIND on when
> > enabling that.
> >
> > I'm more in favour of a #warning on #error in the unwind.c file based on
> > the compiler version rather than not having it on by default. The reason
> > is that people reported performance improvements when compiling the
> > kernel without frame pointers.
> 
> [Sorry, I sent the wrong patch... This is the new one.]

and the wrong ARM kernel list (I changed the Cc line).

> This solution is fine too (even if I still think that changing the
> default setting is better).
> 
> Please, consider the patch in attachment: is it like you would have it ?

You can just send it to Russell's patch system once there are no more
comments on it.

See below for my comments.

> BTW, do we have any list of buggy or not-EABI versions of the gcc
> compiler ?

No, but for non-EABI compilers I think the __ARM_EABI__ check is enough.

> From: Claudio Scordino <claudio@evidence.eu.com>
> Date: Mon, 26 Oct 2009 17:13:18 +0100
> Subject: [PATCH 1/1] Some warnings when compiling ARM unwind support with a buggy or not-EABI compiler.
> 
> ARM unwind support is known to build only with EABI and not-buggy compilers.
> Now we check the compiler and raise a #warning in case of wrong compiler.

Maybe you can clarify a bit that the problem is not unwinding
information but the -fno-frame-pointer option added as a result of 
!CONFIG_FRAME_POINTER.

> Signed-off-by: Claudio Scordino <claudio@evidence.eu.com>
> ---
>  arch/arm/kernel/unwind.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> index 39baf11..47345c1 100644
> --- a/arch/arm/kernel/unwind.c
> +++ b/arch/arm/kernel/unwind.c
> @@ -26,6 +26,15 @@
>   * http://infocenter.arm.com/help/topic/com.arm.doc.subset.swdev.abi/index.html
>   */
>  
> +#if !defined (__ARM_EABI__)
> +#warning Your compiler does not have EABI support.
> +#warning    ARM unwind support it is known to compile only on EABI compilers.

Maybe "ARM unwind is known ... only with ..." (though English is not my
first language).

> +#warning    Change compiler or disable ARM_UNWIND option.
> +#elif (__GNUC__ == 4 && __GNUC_MINOR__ == 2)

Could we assume this for (__GNUC_MINOR__ <= 2)?

> +#warning Your compiler is too buggy; it is known to not compile ARM unwind support.
> +#warning    Change compiler or disable ARM_UNWIND option.
> +#endif
> +
>  #include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/module.h>

-- 
Catalin

Re: [PATCH][RE-SUBMIT] Default setting of the ARM_UNWIND option

[Claudio Scordino]

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

Hi,

    I modified the patch according to your suggestions.

> 
> You can just send it to Russell's patch system once there are no more
> comments on it.

Done. I'm attaching the latest version here too.

Many thanks,

            Claudio

[-- Attachment #2: 0001-ARM-unwind-is-known-to-compile-only-with-EABI-and-no.patch --]
[-- Type: text/x-patch, Size: 1372 bytes --]

From: Claudio Scordino <claudio@evidence.eu.com>
Date: Fri, 30 Oct 2009 11:46:00 +0100
Subject: [PATCH 1/1] ARM unwind is known to compile only with EABI and not-buggy compilers.

ARM unwind is known to compile only with EABI and not-buggy compilers.
The problem is not the unwinding information but the -fno-frame-pointer option added as a result of !CONFIG_FRAME_POINTER.
Now we check the compiler and raise a #warning in case of wrong compiler.


Signed-off-by: Claudio Scordino <claudio@evidence.eu.com>
---
 arch/arm/kernel/unwind.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 39baf11..786ac2b 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -26,6 +26,15 @@
  * http://infocenter.arm.com/help/topic/com.arm.doc.subset.swdev.abi/index.html
  */
 
+#if !defined (__ARM_EABI__)
+#warning Your compiler does not have EABI support.
+#warning    ARM unwind is known to compile only with EABI compilers.
+#warning    Change compiler or disable ARM_UNWIND option.
+#elif (__GNUC__ == 4 && __GNUC_MINOR__ <= 2)
+#warning Your compiler is too buggy; it is known to not compile ARM unwind support.
+#warning    Change compiler or disable ARM_UNWIND option.
+#endif
+
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/module.h>
-- 
1.6.0.4

Re: [PATCH][RE-SUBMIT] Default setting of the ARM_UNWIND option

[Catalin Marinas]

On Fri, 2009-10-30 at 12:11 +0100, Claudio Scordino wrote:
> From: Claudio Scordino <claudio@evidence.eu.com>
> Date: Fri, 30 Oct 2009 11:46:00 +0100
> Subject: [PATCH 1/1] ARM unwind is known to compile only with EABI and
> not-buggy compilers.
> 
> ARM unwind is known to compile only with EABI and not-buggy compilers.
> The problem is not the unwinding information but the
> -fno-frame-pointer option added as a result of !CONFIG_FRAME_POINTER.
> Now we check the compiler and raise a #warning in case of wrong
> compiler.
> 
> 
> Signed-off-by: Claudio Scordino <claudio@evidence.eu.com>

Unless others have objections, here's my ack:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

-- 
Catalin