Add support for unwinding using the dwarf information in compat mode. Using the correct user stack pointer allows perf to record the frames correctly in the native and compat modes.
Note that although the dwarf frame unwinding works ok using libunwind in native mode (on ARMv7 & ARMv8), some changes are required to the libunwind code for the compat mode. Those changes are posted separately on the libunwind mailing list.
Tested on ARMv8 platform with v8 and compat v7 binaries, the latter are statically built.
Signed-off-by: Jean Pihet jean.pihet@linaro.org --- arch/arm64/include/asm/ptrace.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index fbb0020..86d5b54 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -133,7 +133,7 @@ struct pt_regs { (!((regs)->pstate & PSR_F_BIT))
#define user_stack_pointer(regs) \ - ((regs)->sp) + (!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)
/* * Are the current registers suitable for user mode? (used to maintain
Hi Jean,
On Thu, Jan 16, 2014 at 10:45:23AM +0000, Jean Pihet wrote:
Add support for unwinding using the dwarf information in compat mode. Using the correct user stack pointer allows perf to record the frames correctly in the native and compat modes.
Note that although the dwarf frame unwinding works ok using libunwind in native mode (on ARMv7 & ARMv8), some changes are required to the libunwind code for the compat mode. Those changes are posted separately on the libunwind mailing list.
Tested on ARMv8 platform with v8 and compat v7 binaries, the latter are statically built.
I guess it makes sense to include this with your earlier series adding support for compat backtracing?
Signed-off-by: Jean Pihet jean.pihet@linaro.org
arch/arm64/include/asm/ptrace.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index fbb0020..86d5b54 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -133,7 +133,7 @@ struct pt_regs { (!((regs)->pstate & PSR_F_BIT)) #define user_stack_pointer(regs) \
- ((regs)->sp)
- (!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)
In your previous series, compat backtracing is actually split out into a separate function (compat_user_backtrace), so it would be more consistent to have a compat_user_stack_pointer macro, rather than add this check here.
Will
On 16 January 2014 12:56, Will Deacon will.deacon@arm.com wrote:
Hi Jean,
On Thu, Jan 16, 2014 at 10:45:23AM +0000, Jean Pihet wrote:
Add support for unwinding using the dwarf information in compat mode. Using the correct user stack pointer allows perf to record the frames correctly in the native and compat modes.
Note that although the dwarf frame unwinding works ok using libunwind in native mode (on ARMv7 & ARMv8), some changes are required to the libunwind code for the compat mode. Those changes are posted separately on the libunwind mailing list.
Tested on ARMv8 platform with v8 and compat v7 binaries, the latter are statically built.
I guess it makes sense to include this with your earlier series adding support for compat backtracing?
Signed-off-by: Jean Pihet jean.pihet@linaro.org
arch/arm64/include/asm/ptrace.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index fbb0020..86d5b54 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -133,7 +133,7 @@ struct pt_regs { (!((regs)->pstate & PSR_F_BIT))
#define user_stack_pointer(regs) \
((regs)->sp)
(!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)In your previous series, compat backtracing is actually split out into a separate function (compat_user_backtrace), so it would be more consistent to have a compat_user_stack_pointer macro, rather than add this check here.
Do you mean this change instead?
diff --git a/kernel/events/internal.h b/kernel/events/internal.h index 569b2187..9b88d2e 100644 --- a/kernel/events/internal.h +++ b/kernel/events/internal.h @@ -185,7 +185,8 @@ static inline bool arch_perf_have_user_stack_dump(void) return true; }
-#define perf_user_stack_pointer(regs) user_stack_pointer(regs) +#define perf_user_stack_pointer(regs) \ + (!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp) #else static inline bool arch_perf_have_user_stack_dump(void) {
If so let me prepare/test and re-submit this.
Thx! Jean
Will
On Thu, Jan 16, 2014 at 12:26:53PM +0000, Jean Pihet wrote:
On 16 January 2014 12:56, Will Deacon will.deacon@arm.com wrote:
In your previous series, compat backtracing is actually split out into a separate function (compat_user_backtrace), so it would be more consistent to have a compat_user_stack_pointer macro, rather than add this check here.
Do you mean this change instead?
I don't think so...
diff --git a/kernel/events/internal.h b/kernel/events/internal.h index 569b2187..9b88d2e 100644 --- a/kernel/events/internal.h +++ b/kernel/events/internal.h @@ -185,7 +185,8 @@ static inline bool arch_perf_have_user_stack_dump(void) return true; }
-#define perf_user_stack_pointer(regs) user_stack_pointer(regs) +#define perf_user_stack_pointer(regs) \
(!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)
This doesn't belong in core code; compat_user_mode and the fields of regs are arm64-specific. So I suppose you need to rework your original patch to call compat_user_stack_pointer (which we already define in compat.h for arm64) if compat_user_mode(regs)).
The problem there is the inconsistency with respect to the regs argument:
user_stack_pointer(regs) // Returns user stack pointer for regs current_user_stack_pointer() // Returns current user stack pointer compat_user_stack_pointer() // Doesn't take a regs argument!
On top of that, x86 treats those last two functions differently when current is a compat task.
So the simplest thing would be to make compat_user_stack_pointer expand to user_stack_pointer(current_pt_regs()) on arm64 and merge that in with your original patch fixing user_stack_pointer.
Will
Will,
On 16 January 2014 13:57, Will Deacon will.deacon@arm.com wrote:
On Thu, Jan 16, 2014 at 12:26:53PM +0000, Jean Pihet wrote:
On 16 January 2014 12:56, Will Deacon will.deacon@arm.com wrote:
In your previous series, compat backtracing is actually split out into a separate function (compat_user_backtrace), so it would be more consistent to have a compat_user_stack_pointer macro, rather than add this check here.
The compat_user_backtrace function is used to unwind using the frame pointer, it is not used to unwind using the dwarf info (which uses the user stack pointer).
Do you mean this change instead?
I don't think so...
diff --git a/kernel/events/internal.h b/kernel/events/internal.h index 569b2187..9b88d2e 100644 --- a/kernel/events/internal.h +++ b/kernel/events/internal.h @@ -185,7 +185,8 @@ static inline bool arch_perf_have_user_stack_dump(void) return true; }
-#define perf_user_stack_pointer(regs) user_stack_pointer(regs) +#define perf_user_stack_pointer(regs) \
(!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)This doesn't belong in core code; compat_user_mode and the fields of regs are arm64-specific.
Right.
So I suppose you need to rework your original patch to call compat_user_stack_pointer (which we already define in compat.h for arm64) if compat_user_mode(regs)).
The perf core code calls perf_user_stack_pointer(regs) to retrieve the stack pointer, with perf_user_stack_pointer(regs) defined as user_stack_pointer(regs). The problem is that perf is not aware of the compat mode, so every arch has to implement user_stack_pointer(regs) correctly.
For this reason I think the first patch proposal is the right one unless the perf core code is redesigned to handle different ABIs. Do you see a better implementation?
The problem there is the inconsistency with respect to the regs argument:
user_stack_pointer(regs) // Returns user stack pointer for regs current_user_stack_pointer() // Returns current user stack pointer compat_user_stack_pointer() // Doesn't take a regs argument!
On top of that, x86 treats those last two functions differently when current is a compat task.
So the simplest thing would be to make compat_user_stack_pointer expand to user_stack_pointer(current_pt_regs()) on arm64 and merge that in with your original patch fixing user_stack_pointer.
Will
Thx! Jean
Hi Will,
Some more thoughts below
On 16 January 2014 14:47, Jean Pihet jean.pihet@linaro.org wrote:
Will,
On 16 January 2014 13:57, Will Deacon will.deacon@arm.com wrote:
On Thu, Jan 16, 2014 at 12:26:53PM +0000, Jean Pihet wrote:
On 16 January 2014 12:56, Will Deacon will.deacon@arm.com wrote:
In your previous series, compat backtracing is actually split out into a separate function (compat_user_backtrace), so it would be more consistent to have a compat_user_stack_pointer macro, rather than add this check here.
The compat_user_backtrace function is used to unwind using the frame pointer, it is not used to unwind using the dwarf info (which uses the user stack pointer).
Do you mean this change instead?
I don't think so...
diff --git a/kernel/events/internal.h b/kernel/events/internal.h index 569b2187..9b88d2e 100644 --- a/kernel/events/internal.h +++ b/kernel/events/internal.h @@ -185,7 +185,8 @@ static inline bool arch_perf_have_user_stack_dump(void) return true; }
-#define perf_user_stack_pointer(regs) user_stack_pointer(regs) +#define perf_user_stack_pointer(regs) \
(!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)This doesn't belong in core code; compat_user_mode and the fields of regs are arm64-specific.
Right.
So I suppose you need to rework your original patch to call compat_user_stack_pointer (which we already define in compat.h for arm64) if compat_user_mode(regs)).
The perf core code calls perf_user_stack_pointer(regs) to retrieve the stack pointer, with perf_user_stack_pointer(regs) defined as user_stack_pointer(regs). The problem is that perf is not aware of the compat mode, so every arch has to implement user_stack_pointer(regs) correctly.
For this reason I think the first patch proposal is the right one unless the perf core code is redesigned to handle different ABIs. Do you see a better implementation?
The problem there is the inconsistency with respect to the regs argument:
user_stack_pointer(regs) // Returns user stack pointer for regs current_user_stack_pointer() // Returns current user stack pointer compat_user_stack_pointer() // Doesn't take a regs argument!
On top of that, x86 treats those last two functions differently when current is a compat task.
So the simplest thing would be to make compat_user_stack_pointer expand to user_stack_pointer(current_pt_regs()) on arm64 and merge that in with your original patch fixing user_stack_pointer.
I see 2 issues in your proposal:
1) user_stack_pointer(regs) calls compat_user_stack_pointer if compat_user_mode(regs)) and compat_user_stack_pointer expands to user_stack_pointer. I see a circular dependency in the macros.
2) current_pt_regs() returns the current task regs although perf passes a regs struct that had been recorded previously.
Am I missing something?
Thx, Jean
Will
Thx! Jean
On Fri, Jan 17, 2014 at 09:00:09AM +0000, Jean Pihet wrote:
On 16 January 2014 14:47, Jean Pihet jean.pihet@linaro.org wrote:
So the simplest thing would be to make compat_user_stack_pointer expand to user_stack_pointer(current_pt_regs()) on arm64 and merge that in with your original patch fixing user_stack_pointer.
I see 2 issues in your proposal:
- user_stack_pointer(regs) calls compat_user_stack_pointer if
compat_user_mode(regs)) and compat_user_stack_pointer expands to user_stack_pointer. I see a circular dependency in the macros.
Not today it doesn't, so you just need to avoid writing the circular dependency and instead make user_stack_pointer access (regs)->compat_sp instead.
- current_pt_regs() returns the current task regs although perf
passes a regs struct that had been recorded previously.
Yes, but compat_user_stack_pointer doesn't take a regs paramater anyway, so there's no change in behaviour here.
Will
Hi Will,
Here is an updated version of the change, which uses compat_sp at only one place. The drawback is that compat_user_mode is checked when calling compat_user_stack_pointer, which seems unnecessary. Unfortunately the check is not optimized out by the complier as I could check with objdump -S.
What do you think?
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index fda2704..e71f81f 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h @@ -228,7 +228,7 @@ static inline compat_uptr_t ptr_to_compat(void __user *uptr) return (u32)(unsigned long)uptr; }
-#define compat_user_stack_pointer() (current_pt_regs()->compat_sp) +#define compat_user_stack_pointer() (user_stack_pointer(current_pt_regs()))
static inline void __user *arch_compat_alloc_user_space(long len) { diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index fbb0020..86d5b54 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -133,7 +133,7 @@ struct pt_regs { (!((regs)->pstate & PSR_F_BIT))
#define user_stack_pointer(regs) \ - ((regs)->sp) + (!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)
/* * Are the current registers suitable for user mode? (used to maintain
Regards, Jean
On 17 January 2014 11:07, Will Deacon will.deacon@arm.com wrote:
On Fri, Jan 17, 2014 at 09:00:09AM +0000, Jean Pihet wrote:
On 16 January 2014 14:47, Jean Pihet jean.pihet@linaro.org wrote:
So the simplest thing would be to make compat_user_stack_pointer expand to user_stack_pointer(current_pt_regs()) on arm64 and merge that in with your original patch fixing user_stack_pointer.
I see 2 issues in your proposal:
- user_stack_pointer(regs) calls compat_user_stack_pointer if
compat_user_mode(regs)) and compat_user_stack_pointer expands to user_stack_pointer. I see a circular dependency in the macros.
Not today it doesn't, so you just need to avoid writing the circular dependency and instead make user_stack_pointer access (regs)->compat_sp instead.
- current_pt_regs() returns the current task regs although perf
passes a regs struct that had been recorded previously.
Yes, but compat_user_stack_pointer doesn't take a regs paramater anyway, so there's no change in behaviour here.
Will
On Mon, Jan 20, 2014 at 05:05:14PM +0000, Jean Pihet wrote:
Hi Will,
Here is an updated version of the change, which uses compat_sp at only one place. The drawback is that compat_user_mode is checked when calling compat_user_stack_pointer, which seems unnecessary. Unfortunately the check is not optimized out by the complier as I could check with objdump -S.
What do you think?
I think that's cleaner and really wouldn't worry about the couple of extra instructions.
Cheers,
Will
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index fda2704..e71f81f 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h @@ -228,7 +228,7 @@ static inline compat_uptr_t ptr_to_compat(void __user *uptr) return (u32)(unsigned long)uptr; }
-#define compat_user_stack_pointer() (current_pt_regs()->compat_sp) +#define compat_user_stack_pointer() (user_stack_pointer(current_pt_regs()))
static inline void __user *arch_compat_alloc_user_space(long len) { diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h index fbb0020..86d5b54 100644 --- a/arch/arm64/include/asm/ptrace.h +++ b/arch/arm64/include/asm/ptrace.h @@ -133,7 +133,7 @@ struct pt_regs { (!((regs)->pstate & PSR_F_BIT))
#define user_stack_pointer(regs) \
((regs)->sp)
(!compat_user_mode(regs)) ? ((regs)->sp) : ((regs)->compat_sp)/*
- Are the current registers suitable for user mode? (used to maintain
linaro-kernel@lists.linaro.org
-
Jean Pihet -
Will Deacon