[PATCH] Kconfig.debug: Add FRAME_POINTER anti-dependency for ARC

[PATCH] Kconfig.debug: Add FRAME_POINTER anti-dependency for ARC

[Vineet Gupta]

Frame pointer on ARC doesn't serve the conventional purpose of stack
unwinding due to the typical way ABI designates it's usage.
Thus it's explicit usage on ARC is discouraged (gcc is free to use it,
for some tricky stack frames even if -fomit-frame-pointer).

Hence no point enabling it for ARC.

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: "Paul E. McKenney" <paul.mckenney@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Michel Lespinasse <walken@google.com>
Cc: linux-kernel@vger.kernel.org
---
 lib/Kconfig.debug | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 1501aa5..c971f3a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -908,7 +908,7 @@ config LOCKDEP
 	bool
 	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
 	select STACKTRACE
-	select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND && !S390 && !MICROBLAZE
+	select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND && !S390 && !MICROBLAZE && !ARC
 	select KALLSYMS
 	select KALLSYMS_ALL
 
@@ -1347,7 +1347,7 @@ config FAULT_INJECTION_STACKTRACE_FILTER
 	depends on FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT
 	depends on !X86_64
 	select STACKTRACE
-	select FRAME_POINTER if !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM_UNWIND
+	select FRAME_POINTER if !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM_UNWIND && !ARC
 	help
 	  Provide stacktrace filter for fault-injection capabilities
 
@@ -1357,7 +1357,7 @@ config LATENCYTOP
 	depends on DEBUG_KERNEL
 	depends on STACKTRACE_SUPPORT
 	depends on PROC_FS
-	select FRAME_POINTER if !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM_UNWIND
+	select FRAME_POINTER if !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM_UNWIND && !ARC
 	select KALLSYMS
 	select KALLSYMS_ALL
 	select STACKTRACE
-- 
1.8.1.2

Re: [PATCH] Kconfig.debug: Add FRAME_POINTER anti-dependency for ARC

[Vineet Gupta]

Ping ?

-Vineet

On 08/27/2013 02:01 PM, Vineet Gupta wrote:
> Frame pointer on ARC doesn't serve the conventional purpose of stack
> unwinding due to the typical way ABI designates it's usage.
> Thus it's explicit usage on ARC is discouraged (gcc is free to use it,
> for some tricky stack frames even if -fomit-frame-pointer).
> 
> Hence no point enabling it for ARC.
> 
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: "Paul E. McKenney" <paul.mckenney@linaro.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Michel Lespinasse <walken@google.com>
> Cc: linux-kernel@vger.kernel.org
> ---
>  lib/Kconfig.debug | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 1501aa5..c971f3a 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -908,7 +908,7 @@ config LOCKDEP
>  	bool
>  	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
>  	select STACKTRACE
> -	select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND && !S390 && !MICROBLAZE
> +	select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND && !S390 && !MICROBLAZE && !ARC
>  	select KALLSYMS
>  	select KALLSYMS_ALL
>  
> @@ -1347,7 +1347,7 @@ config FAULT_INJECTION_STACKTRACE_FILTER
>  	depends on FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT
>  	depends on !X86_64
>  	select STACKTRACE
> -	select FRAME_POINTER if !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM_UNWIND
> +	select FRAME_POINTER if !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM_UNWIND && !ARC
>  	help
>  	  Provide stacktrace filter for fault-injection capabilities
>  
> @@ -1357,7 +1357,7 @@ config LATENCYTOP
>  	depends on DEBUG_KERNEL
>  	depends on STACKTRACE_SUPPORT
>  	depends on PROC_FS
> -	select FRAME_POINTER if !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM_UNWIND
> +	select FRAME_POINTER if !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM_UNWIND && !ARC
>  	select KALLSYMS
>  	select KALLSYMS_ALL
>  	select STACKTRACE
> 

Re: [PATCH] Kconfig.debug: Add FRAME_POINTER anti-dependency for ARC

[Dave Hansen]

On 08/27/2013 01:31 AM, Vineet Gupta wrote:
> Frame pointer on ARC doesn't serve the conventional purpose of stack
> unwinding due to the typical way ABI designates it's usage.
> Thus it's explicit usage on ARC is discouraged (gcc is free to use it,
> for some tricky stack frames even if -fomit-frame-pointer)
...
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 1501aa5..c971f3a 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -908,7 +908,7 @@ config LOCKDEP
>  	bool
>  	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
>  	select STACKTRACE
> -	select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND && !S390 && !MICROBLAZE
> +	select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND && !S390 && !MICROBLAZE && !ARC
>  	select KALLSYMS
>  	select KALLSYMS_ALL

I assume you're sending this my way since getmaintainer.pl has me tagged
I moved a bunch of code in there. :)

The Kconfig.debug stuff has no real maintainer.  It would probably be OK
if you just stick this in your architecture's next git pull request.

Although, it would be nice if someone, at some point, actually
abstracted that out so that the individual architectures could take care
of this without editing the main files:

# Kconfig.debug:

config ARCH_FRAME_POINTER_UNAVAILABLE
	def_bool y
...
select FRAME_POINTER if !ARCH_FRAME_POINTER_UNAVAILABLE

# arch/.../Kconfig

select ARCH_FRAME_POINTER_UNAVAILABLE

Re: [PATCH] Kconfig.debug: Add FRAME_POINTER anti-dependency for ARC

[Vineet Gupta]

On 08/29/2013 08:48 PM, Dave Hansen wrote:
>
> I assume you're sending this my way since getmaintainer.pl has me tagged
> I moved a bunch of code in there. :)

Indeed :-)

> The Kconfig.debug stuff has no real maintainer.  It would probably be OK
> if you just stick this in your architecture's next git pull request.

Will do, thx.

> Although, it would be nice if someone, at some point, actually
> abstracted that out so that the individual architectures could take care
> of this without editing the main files:
>
> # Kconfig.debug:
>
> config ARCH_FRAME_POINTER_UNAVAILABLE
> 	def_bool y

def_bool n

> ...
> select FRAME_POINTER if !ARCH_FRAME_POINTER_UNAVAILABLE
>
> # arch/.../Kconfig
>
> select ARCH_FRAME_POINTER_UNAVAILABLE

Sure, I was thinking the same. I'll send out a patchset soon, although next week
is gonna be tight due to upcoming merge window.

-Vineet

Re: [PATCH] Kconfig.debug: Add FRAME_POINTER anti-dependency for ARC

[Vineet Gupta]

[+linux-arch and other arch maintainers]

On 08/29/2013 08:48 PM, Dave Hansen wrote:
> On 08/27/2013 01:31 AM, Vineet Gupta wrote:
>> Frame pointer on ARC doesn't serve the conventional purpose of stack
>> unwinding due to the typical way ABI designates it's usage.
>> Thus it's explicit usage on ARC is discouraged (gcc is free to use it,
>> for some tricky stack frames even if -fomit-frame-pointer)
> ...
>> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
>> index 1501aa5..c971f3a 100644
>> --- a/lib/Kconfig.debug
>> +++ b/lib/Kconfig.debug
>> @@ -908,7 +908,7 @@ config LOCKDEP
>>  	bool
>>  	depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
>>  	select STACKTRACE
>> -	select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND && !S390 && !MICROBLAZE
>> +	select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND && !S390 && !MICROBLAZE && !ARC
>>  	select KALLSYMS
>>  	select KALLSYMS_ALL
> 
> I assume you're sending this my way since getmaintainer.pl has me tagged
> I moved a bunch of code in there. :)
> 
> The Kconfig.debug stuff has no real maintainer.  It would probably be OK
> if you just stick this in your architecture's next git pull request.
> 
> Although, it would be nice if someone, at some point, actually
> abstracted that out so that the individual architectures could take care
> of this without editing the main files:
> 
> # Kconfig.debug:
> 
> config ARCH_FRAME_POINTER_UNAVAILABLE
> 	def_bool y
> ...
> select FRAME_POINTER if !ARCH_FRAME_POINTER_UNAVAILABLE
> 
> # arch/.../Kconfig
> 
> select ARCH_FRAME_POINTER_UNAVAILABLE
> 
> 

I thought about this a bit. So adding ARCH_FRAME_POINTER_UNAVAILABLE does help
cleanup the anti-dependency list for say LATENCY_TOP for (MIPS, PPC, S390,
MICROBLAZE, ARM_UNWIND, ARC), but we are still stuck with keeping both
ARCH_WANT_FRAME_POINTERS and ARCH_FRAME_POINTER_UNAVAILABLE.

If we had ARCH_FRAME_POINTER_UNAVAILABLE (def_bool n), we could potentially remove
ARCH_FRAME_POINTER too:

1. arches which explicitly select ARCH_FRAME_POINTER (xtensa, parisc, arm64, x86,
unicore32, tile) could just drop that select.

2. Others which add themselves to config FRAME_POINTER depends on (CRIS, M68K,
FRV, UML, AVR32, SUPERH, BLACKFIN, MN10300, METAG) can simply be removed form that
list.

3. Other who want to inhibit FP obviously select it (MIPS, PPC, S390, MICROBLAZE,
ARM_UNWIND, ARC)

The issue is some (sparc, c6x...) which are neither in #1 or #2, and not present
in anti-dependency list either. e.g. With sparc64_defconfig FP is not present, but
if I enable LATENCY_TOP, FP is enabled. For such cases, what do we make default ?

This seemed so trivial to do to begin with :-)

-Vineet

Re: [PATCH] Kconfig.debug: Add FRAME_POINTER anti-dependency for ARC

[Dave Hansen]

On 08/30/2013 12:48 AM, Vineet Gupta wrote:
> If we had ARCH_FRAME_POINTER_UNAVAILABLE (def_bool n), we could potentially remove
> ARCH_FRAME_POINTER too:
> 
> 1. arches which explicitly select ARCH_FRAME_POINTER (xtensa, parisc, arm64, x86,
> unicore32, tile) could just drop that select.
> 
> 2. Others which add themselves to config FRAME_POINTER depends on (CRIS, M68K,
> FRV, UML, AVR32, SUPERH, BLACKFIN, MN10300, METAG) can simply be removed form that
> list.
> 
> 3. Other who want to inhibit FP obviously select it (MIPS, PPC, S390, MICROBLAZE,
> ARM_UNWIND, ARC)

That all seems sane to me.

> The issue is some (sparc, c6x...) which are neither in #1 or #2, and not present
> in anti-dependency list either. e.g. With sparc64_defconfig FP is not present, but
> if I enable LATENCY_TOP, FP is enabled. For such cases, what do we make default ?

You can list multiple defaults if you want, or have them depend on other
config variables:

config FOO
	default BAR

or

config FOO
	default y if BAR
	default n if BAZ

ARCH_FRAME_POINTER_UNAVAILABLE doesn't make much sense if
FRAME_POINTER=n, right?  You can have it just plain depend on
FRAME_POINTER, I think.

Re: [PATCH] Kconfig.debug: Add FRAME_POINTER anti-dependency for ARC

[Vineet Gupta]

On 08/30/2013 08:50 PM, Dave Hansen wrote:
> On 08/30/2013 12:48 AM, Vineet Gupta wrote:
>> If we had ARCH_FRAME_POINTER_UNAVAILABLE (def_bool n), we could potentially remove
>> ARCH_FRAME_POINTER too:

>> The issue is some (sparc, c6x...) which are neither in #1 or #2, and not present
>> in anti-dependency list either. e.g. With sparc64_defconfig FP is not present, but
>> if I enable LATENCY_TOP, FP is enabled. For such cases, what do we make default ?
> 
> You can list multiple defaults if you want, or have them depend on other
> config variables:
> 
> config FOO
> 	default BAR
> 
> or
> 
> config FOO
> 	default y if BAR
> 	default n if BAZ
> 
> ARCH_FRAME_POINTER_UNAVAILABLE doesn't make much sense if
> FRAME_POINTER=n, right?  You can have it just plain depend on
> FRAME_POINTER, I think.


I think I was not very clear with the problem description.

With a defbool 'n', FP will be by default enabled and arches not interested in FP
will select ARCH_FRAME_POINTER_UNAVAILABLE. e.g. SPARC, so far so good.

That however means that LATENCYTOP enabled in sparc64_defconfig will now build
with !FP, whereas as of today it enables FP (and SPARC code must be OK with FP
enabling in this config). So, we are changing semantics here, which might still be
OK, but I'll only trust arch maintainers' NOD. So the change is not just
mechanical from that perspective.

My point is, before I cook the patch-set we must be in agreement to this
semantical change.

-Vineet

Re: [PATCH] Kconfig.debug: Add FRAME_POINTER anti-dependency for ARC

[Vineet Gupta]

On 08/30/2013 08:50 PM, Dave Hansen wrote:
> On 08/30/2013 12:48 AM, Vineet Gupta wrote:
>> If we had ARCH_FRAME_POINTER_UNAVAILABLE (def_bool n), we could potentially remove
>> ARCH_FRAME_POINTER too:

>> The issue is some (sparc, c6x...) which are neither in #1 or #2, and not present
>> in anti-dependency list either. e.g. With sparc64_defconfig FP is not present, but
>> if I enable LATENCY_TOP, FP is enabled. For such cases, what do we make default ?
> 
> You can list multiple defaults if you want, or have them depend on other
> config variables:
> 
> config FOO
> 	default BAR
> 
> or
> 
> config FOO
> 	default y if BAR
> 	default n if BAZ
> 
> ARCH_FRAME_POINTER_UNAVAILABLE doesn't make much sense if
> FRAME_POINTER=n, right?  You can have it just plain depend on
> FRAME_POINTER, I think.


I think I was not very clear with the problem description.

With a defbool 'n', FP will be by default enabled and arches not interested in FP
will select ARCH_FRAME_POINTER_UNAVAILABLE. e.g. SPARC, so far so good.

That however means that LATENCYTOP enabled in sparc64_defconfig will now build
with !FP, whereas as of today it enables FP (and SPARC code must be OK with FP
enabling in this config). So, we are changing semantics here, which might still be
OK, but I'll only trust arch maintainers' NOD. So the change is not just
mechanical from that perspective.

My point is, before I cook the patch-set we must be in agreement to this
semantical change.

-Vineet

Re: [PATCH] Kconfig.debug: Add FRAME_POINTER anti-dependency for ARC

[Gilad Ben-Yossef]

Hi,

On Tue, Aug 27, 2013 at 11:31 AM, Vineet Gupta
<Vineet.Gupta1@synopsys.com> wrote:
>
> Frame pointer on ARC doesn't serve the conventional purpose of stack
> unwinding due to the typical way ABI designates it's usage.

More out of curiosity to understand the platform better than actual
review - can you explain a little what
you meant by this statement?

Is this statement because of the use of write back mode with ld/st to
or not related?

Thanks,
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker
gilad@benyossef.com
Israel Cell: +972-52-8260388
US Cell: +1-973-8260388
http://benyossef.com

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH] Kconfig.debug: Add FRAME_POINTER anti-dependency for ARC

[Vineet Gupta]

Hi Gilad,

On 09/02/2013 02:33 PM, Gilad Ben-Yossef wrote:
> Hi,
>
> On Tue, Aug 27, 2013 at 11:31 AM, Vineet Gupta
> <Vineet.Gupta1@synopsys.com> wrote:
>> Frame pointer on ARC doesn't serve the conventional purpose of stack
>> unwinding due to the typical way ABI designates it's usage.
> More out of curiosity to understand the platform better than actual
> review - can you explain a little what
> you meant by this statement?
>
> Is this statement because of the use of write back mode with ld/st to
> or not related?

No - this is not related to to any LD/ST addressing/wb modes.

ARCompact ISA has 2 stack related registers, SP (top) and FP (Base)

A typical frame pointer-ish ABI would do sometime like this
1. push BLINK (return address)
2. push FP
3. FP = SP
4. push callee-regs
5. allocate stack for local vars etc
...

Note that beyond #3, FP remains constant while this function is in scope. Thus
from anywhere inside the function, [FP, 4] always has caller's PC and [FP, 0]
always has caller's FP. This makes it possible to retract back to caller and from
there to it's caller and so on.

However the ARC gcc ABI - set in stone many years ago doesn't do this. It does 1,
4, 2, 3...

With FP no longer having base address of current call, it can't be used to get
FP/BLINK of prev frame so from stack unwinding perspective, there's no point in
using FP for stack frames. More importantly, FP usage bloats function
prologue/epilogue - adds extra stack ops, .... hence it's usage in general is
discouraged. gcc though is free to summon it's usage for typical stack frames (var
sized arrays etc).

Hence the reason we don't ever build ARC with FP and need for this patch.

HTH,
-Vineet