Opened 9 years ago

Closed 9 years ago

#6139 closed enhancement (fixed)

[PATCH] ppc: Implement arch_debug_get_caller

Reported by: andreasf Owned by: axeld
Priority: normal Milestone: R1
Component: System/Kernel Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: PowerPC

Description

Some low hanging fruit: Resolve a TODO, code is adapted from x86.

Priority: low

Attachments (3)

0001-Implement-arch_debug_get_caller.patch (860 bytes) - added by andreasf 9 years ago.
proposed patch
0001-Implement-arch_debug_get_caller.2.patch (874 bytes) - added by andreasf 9 years ago.
proposed patch
0002-Exclude-stack_trace-function-from-stack-trace.patch (1.1 KB) - added by andreasf 9 years ago.
proposed patch: exclude stack_trace from stack trace

Download all attachments as: .zip

Change History (9)

Changed 9 years ago by andreasf

proposed patch

comment:1 Changed 9 years ago by andreasf

Has a Patch: set

comment:2 Changed 9 years ago by bonefish

I don't think that this returns the correct result. Unlike on x86 the return address is not stored in the own stack frame, but in the caller's stack frame.

Note that the same issue might ail the stack trace code.

comment:3 Changed 9 years ago by andreasf

Has a Patch: unset

Changed 9 years ago by andreasf

proposed patch

comment:4 Changed 9 years ago by andreasf

Has a Patch: set

Changed 9 years ago by andreasf

proposed patch: exclude stack_trace from stack trace

comment:5 in reply to:  2 ; Changed 9 years ago by andreasf

Right, it would've returned the requesting function as caller.

As can be seen in #6129, stack_trace itself is included in the stack trace. This is remedied accordingly in patch 2.

Ideally I would like to see everything including panic vanish from the regular stack trace, but that would seem to require major kernel debugger API refactoring, pushing the callsite of debug_call_with_fault_handler to arch-specific code to strip known-preceding frames.
Inside ppc it might be feasible to split stack_trace in one function with stack_frame* argument and a wrapper with the present int argc, char** argv args to handle the evaluate_debug_command case.

comment:6 in reply to:  5 Changed 9 years ago by bonefish

Resolution: fixed
Status: newclosed

Replying to andreasf:

Right, it would've returned the requesting function as caller.

As can be seen in #6129, stack_trace itself is included in the stack trace. This is remedied accordingly in patch 2.

Patches applied in hrev37032 and hrev37033.

Ideally I would like to see everything including panic vanish from the regular stack trace, but that would seem to require major kernel debugger API refactoring, pushing the callsite of debug_call_with_fault_handler to arch-specific code to strip known-preceding frames.

Actually it can be done significantly easier. If you look at enter_kernel_debugger() and debug_trap_cpu_in_kdl(), you'll see that they call arch_debug_save_registers(), which stores the registers state at that point (not implemented for PPC yet). The x86 debug code already uses that for stack traces on other CPUs. I haven't enabled it for the current CPU yet mostly out of concern that any problem in that code would totally break stack traces, though I guess that's really a non-issue. That would already simplify stack traces somewhat. The remaining superfluous stack frames could be skipped, e.g. by passing the number of those stack traces to arch_debug_save_registers() or saving it directly in the structure. Alternatively the outmost kernel debugger function (panic() or kernel_debugger()) could get its caller, which would be stored and serve as a marker for skipping stack frames. Or finally the complete range of kernel debugger code could be skipped at the beginning of the stack trace.

Inside ppc it might be feasible to split stack_trace in one function with stack_frame* argument and a wrapper with the present int argc, char** argv args to handle the evaluate_debug_command case.

The same goes for x86, too. OTOH, arch_debug_stack_trace() is a kernel debugger function. Printing the same stack trace as the bt should be the desired behavior anyway. ATM the stack trace printed automatically on a panic differs from the one bt prints, which is seriously annoying, because it also doesn't match the call stack frame numbering.

Note: See TracTickets for help on using tickets.