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

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