>>> Andi Kleen <[email protected]> 30.08.06 17:40 >>>
>On Wednesday 30 August 2006 17:28, Jan Beulich wrote:
>> >Hmm, yes. Sigh, so it's either gcc changes or binutils changes, and none
>> >of that can be relied upon, except someone has a better idea to identify
>> >signal frames with high enough assurance to be sensible for debugging.
>>
>> The only alternative idea I have is to take into consideration the location
>> of the return address: if it's at the default location (top of call frame),
>> then assume this is a normal frame, in all other cases assume it's an
>> interrupt/exception one. This will probably get a few cases wrong
>> (where the return address is being played with), but it might be better
>> than the current situation. Andi?
>
>Fine by me.
>
>At least it will likely be less controversal than changing all the noreturns :)
Here's a patch, including all that I think is necessary once we selectively
enable (auto-detect) CONFIG_AS_CFI_SIGNAL_FRAME for newer binutils.
It doesn't include adjustment of the printed address (i.e. for the original
example, it'd still be kernel_math_error+0 that gets displayed, but the
unwinder wouldn't get confused anymore.
Signed-off-by: Jan Beulich <[email protected]>
--- linux-2.6.18-rc6/arch/i386/kernel/entry.S 2006-09-11 17:18:19.000000000 +0200
+++ 2.6.18-rc6-unwind-adjust-caller-pc/arch/i386/kernel/entry.S 2006-09-11 15:26:14.000000000 +0200
@@ -176,18 +176,21 @@ VM_MASK = 0x00020000
#define RING0_INT_FRAME \
CFI_STARTPROC simple;\
+ CFI_SIGNAL_FRAME;\
CFI_DEF_CFA esp, 3*4;\
/*CFI_OFFSET cs, -2*4;*/\
CFI_OFFSET eip, -3*4
#define RING0_EC_FRAME \
CFI_STARTPROC simple;\
+ CFI_SIGNAL_FRAME;\
CFI_DEF_CFA esp, 4*4;\
/*CFI_OFFSET cs, -2*4;*/\
CFI_OFFSET eip, -3*4
#define RING0_PTREGS_FRAME \
CFI_STARTPROC simple;\
+ CFI_SIGNAL_FRAME;\
CFI_DEF_CFA esp, OLDESP-EBX;\
/*CFI_OFFSET cs, CS-OLDESP;*/\
CFI_OFFSET eip, EIP-OLDESP;\
@@ -263,6 +266,7 @@ need_resched:
# sysenter call handler stub
ENTRY(sysenter_entry)
CFI_STARTPROC simple
+ CFI_SIGNAL_FRAME
CFI_DEF_CFA esp, 0
CFI_REGISTER esp, ebp
movl TSS_sysenter_esp0(%esp),%esp
--- linux-2.6.18-rc6/arch/x86_64/ia32/ia32entry.S 2006-09-11 17:18:24.000000000 +0200
+++ 2.6.18-rc6-unwind-adjust-caller-pc/arch/x86_64/ia32/ia32entry.S 2006-09-11 15:28:10.000000000 +0200
@@ -71,6 +71,7 @@
*/
ENTRY(ia32_sysenter_target)
CFI_STARTPROC32 simple
+ CFI_SIGNAL_FRAME
CFI_DEF_CFA rsp,0
CFI_REGISTER rsp,rbp
swapgs
@@ -186,6 +187,7 @@ ENDPROC(ia32_sysenter_target)
*/
ENTRY(ia32_cstar_target)
CFI_STARTPROC32 simple
+ CFI_SIGNAL_FRAME
CFI_DEF_CFA rsp,PDA_STACKOFFSET
CFI_REGISTER rip,rcx
/*CFI_REGISTER rflags,r11*/
@@ -293,6 +295,7 @@ ia32_badarg:
ENTRY(ia32_syscall)
CFI_STARTPROC simple
+ CFI_SIGNAL_FRAME
CFI_DEF_CFA rsp,SS+8-RIP
/*CFI_REL_OFFSET ss,SS-RIP*/
CFI_REL_OFFSET rsp,RSP-RIP
@@ -370,6 +373,7 @@ ENTRY(ia32_ptregs_common)
popq %r11
CFI_ENDPROC
CFI_STARTPROC32 simple
+ CFI_SIGNAL_FRAME
CFI_DEF_CFA rsp,SS+8-ARGOFFSET
CFI_REL_OFFSET rax,RAX-ARGOFFSET
CFI_REL_OFFSET rcx,RCX-ARGOFFSET
--- linux-2.6.18-rc6/arch/x86_64/kernel/entry.S 2006-09-11 17:18:24.000000000 +0200
+++ 2.6.18-rc6-unwind-adjust-caller-pc/arch/x86_64/kernel/entry.S 2006-09-11 15:26:47.000000000 +0200
@@ -115,6 +115,7 @@
.macro CFI_DEFAULT_STACK start=1
.if \start
CFI_STARTPROC simple
+ CFI_SIGNAL_FRAME
CFI_DEF_CFA rsp,SS+8
.else
CFI_DEF_CFA_OFFSET SS+8
@@ -199,6 +200,7 @@ END(ret_from_fork)
ENTRY(system_call)
CFI_STARTPROC simple
+ CFI_SIGNAL_FRAME
CFI_DEF_CFA rsp,PDA_STACKOFFSET
CFI_REGISTER rip,rcx
/*CFI_REGISTER rflags,r11*/
@@ -316,6 +318,7 @@ END(system_call)
*/
ENTRY(int_ret_from_sys_call)
CFI_STARTPROC simple
+ CFI_SIGNAL_FRAME
CFI_DEF_CFA rsp,SS+8-ARGOFFSET
/*CFI_REL_OFFSET ss,SS-ARGOFFSET*/
CFI_REL_OFFSET rsp,RSP-ARGOFFSET
@@ -476,6 +479,7 @@ END(stub_rt_sigreturn)
*/
.macro _frame ref
CFI_STARTPROC simple
+ CFI_SIGNAL_FRAME
CFI_DEF_CFA rsp,SS+8-\ref
/*CFI_REL_OFFSET ss,SS-\ref*/
CFI_REL_OFFSET rsp,RSP-\ref
--- linux-2.6.18-rc6/include/asm-i386/dwarf2.h 2006-09-11 17:18:42.000000000 +0200
+++ 2.6.18-rc6-unwind-adjust-caller-pc/include/asm-i386/dwarf2.h 2006-09-11 15:16:45.000000000 +0200
@@ -28,6 +28,11 @@
#define CFI_RESTORE .cfi_restore
#define CFI_REMEMBER_STATE .cfi_remember_state
#define CFI_RESTORE_STATE .cfi_restore_state
+#ifdef CONFIG_AS_CFI_SIGNAL_FRAME
+#define CFI_SIGNAL_FRAME .cfi_signal_frame
+#else
+#define CFI_SIGNAL_FRAME
+#endif
#else
@@ -48,6 +53,7 @@
#define CFI_RESTORE ignore
#define CFI_REMEMBER_STATE ignore
#define CFI_RESTORE_STATE ignore
+#define CFI_SIGNAL_FRAME ignore
#endif
--- linux-2.6.18-rc6/include/asm-i386/unwind.h 2006-09-11 17:18:42.000000000 +0200
+++ 2.6.18-rc6-unwind-adjust-caller-pc/include/asm-i386/unwind.h 2006-09-11 16:10:20.000000000 +0200
@@ -18,6 +18,7 @@ struct unwind_frame_info
{
struct pt_regs regs;
struct task_struct *task;
+ unsigned call_frame:1;
};
#define UNW_PC(frame) (frame)->regs.eip
@@ -42,6 +43,10 @@ struct unwind_frame_info
PTREGS_INFO(edi), \
PTREGS_INFO(eip)
+#define UNW_DEFAULT_RA(raItem, dataAlign) \
+ ((raItem).where == Memory && \
+ !((raItem).value * (dataAlign) + 4))
+
static inline void arch_unw_init_frame_info(struct unwind_frame_info *info,
/*const*/ struct pt_regs *regs)
{
--- linux-2.6.18-rc6/include/asm-x86_64/dwarf2.h 2006-09-11 17:18:44.000000000 +0200
+++ 2.6.18-rc6-unwind-adjust-caller-pc/include/asm-x86_64/dwarf2.h 2006-09-11 15:17:13.000000000 +0200
@@ -28,6 +28,11 @@
#define CFI_REMEMBER_STATE .cfi_remember_state
#define CFI_RESTORE_STATE .cfi_restore_state
#define CFI_UNDEFINED .cfi_undefined
+#ifdef CONFIG_AS_CFI_SIGNAL_FRAME
+#define CFI_SIGNAL_FRAME .cfi_signal_frame
+#else
+#define CFI_SIGNAL_FRAME
+#endif
#else
@@ -45,6 +50,7 @@
#define CFI_REMEMBER_STATE #
#define CFI_RESTORE_STATE #
#define CFI_UNDEFINED #
+#define CFI_SIGNAL_FRAME #
#endif
--- linux-2.6.18-rc6/include/asm-x86_64/unwind.h 2006-09-11 17:18:44.000000000 +0200
+++ 2.6.18-rc6-unwind-adjust-caller-pc/include/asm-x86_64/unwind.h 2006-09-11 16:10:29.000000000 +0200
@@ -18,6 +18,7 @@ struct unwind_frame_info
{
struct pt_regs regs;
struct task_struct *task;
+ unsigned call_frame:1;
};
#define UNW_PC(frame) (frame)->regs.rip
@@ -57,6 +58,10 @@ struct unwind_frame_info
PTREGS_INFO(r15), \
PTREGS_INFO(rip)
+#define UNW_DEFAULT_RA(raItem, dataAlign) \
+ ((raItem).where == Memory && \
+ !((raItem).value * (dataAlign) + 8))
+
static inline void arch_unw_init_frame_info(struct unwind_frame_info *info,
/*const*/ struct pt_regs *regs)
{
--- linux-2.6.18-rc6/kernel/unwind.c 2006-09-11 17:18:46.000000000 +0200
+++ 2.6.18-rc6-unwind-adjust-caller-pc/kernel/unwind.c 2006-09-11 16:44:02.000000000 +0200
@@ -603,6 +603,7 @@ int unwind(struct unwind_frame_info *fra
#define FRAME_REG(r, t) (((t *)frame)[reg_info[r].offs])
const u32 *fde = NULL, *cie = NULL;
const u8 *ptr = NULL, *end = NULL;
+ unsigned long pc = UNW_PC(frame) - frame->call_frame;
unsigned long startLoc = 0, endLoc = 0, cfa;
unsigned i;
signed ptrType = -1;
@@ -612,7 +613,7 @@ int unwind(struct unwind_frame_info *fra
if (UNW_PC(frame) == 0)
return -EINVAL;
- if ((table = find_table(UNW_PC(frame))) != NULL
+ if ((table = find_table(pc)) != NULL
&& !(table->size & (sizeof(*fde) - 1))) {
unsigned long tableSize = table->size;
@@ -647,7 +648,7 @@ int unwind(struct unwind_frame_info *fra
ptrType & DW_EH_PE_indirect
? ptrType
: ptrType & (DW_EH_PE_FORM|DW_EH_PE_signed));
- if (UNW_PC(frame) >= startLoc && UNW_PC(frame) < endLoc)
+ if (pc >= startLoc && pc < endLoc)
break;
cie = NULL;
}
@@ -657,16 +658,28 @@ int unwind(struct unwind_frame_info *fra
state.cieEnd = ptr; /* keep here temporarily */
ptr = (const u8 *)(cie + 2);
end = (const u8 *)(cie + 1) + *cie;
+ frame->call_frame = 1;
if ((state.version = *ptr) != 1)
cie = NULL; /* unsupported version */
else if (*++ptr) {
/* check if augmentation size is first (and thus present) */
if (*ptr == 'z') {
- /* check for ignorable (or already handled)
- * nul-terminated augmentation string */
- while (++ptr < end && *ptr)
- if (strchr("LPR", *ptr) == NULL)
+ while (++ptr < end && *ptr) {
+ switch(*ptr) {
+ /* check for ignorable (or already handled)
+ * nul-terminated augmentation string */
+ case 'L':
+ case 'P':
+ case 'R':
+ continue;
+ case 'S':
+ frame->call_frame = 0;
+ continue;
+ default:
break;
+ }
+ break;
+ }
}
if (ptr >= end || *ptr)
cie = NULL;
@@ -755,7 +768,7 @@ int unwind(struct unwind_frame_info *fra
state.org = startLoc;
memcpy(&state.cfa, &badCFA, sizeof(state.cfa));
/* process instructions */
- if (!processCFI(ptr, end, UNW_PC(frame), ptrType, &state)
+ if (!processCFI(ptr, end, pc, ptrType, &state)
|| state.loc > endLoc
|| state.regs[retAddrReg].where == Nowhere
|| state.cfa.reg >= ARRAY_SIZE(reg_info)
@@ -763,6 +776,11 @@ int unwind(struct unwind_frame_info *fra
|| state.cfa.offs % sizeof(unsigned long))
return -EIO;
/* update frame */
+#ifndef CONFIG_AS_CFI_SIGNAL_FRAME
+ if(frame->call_frame
+ && !UNW_DEFAULT_RA(state.regs[retAddrReg], state.dataAlign))
+ frame->call_frame = 0;
+#endif
cfa = FRAME_REG(state.cfa.reg, unsigned long) + state.cfa.offs;
startLoc = min((unsigned long)UNW_SP(frame), cfa);
endLoc = max((unsigned long)UNW_SP(frame), cfa);
@@ -866,6 +884,7 @@ int unwind_init_frame_info(struct unwind
/*const*/ struct pt_regs *regs)
{
info->task = tsk;
+ info->call_frame = 0;
arch_unw_init_frame_info(info, regs);
return 0;
@@ -879,6 +898,7 @@ int unwind_init_blocked(struct unwind_fr
struct task_struct *tsk)
{
info->task = tsk;
+ info->call_frame = 0;
arch_unw_init_blocked(info);
return 0;
@@ -894,6 +914,7 @@ int unwind_init_running(struct unwind_fr
void *arg)
{
info->task = current;
+ info->call_frame = 0;
return arch_unwind_init_running(info, callback, arg);
}
On Monday 11 September 2006 17:37, Jan Beulich wrote:
> >>> Andi Kleen <[email protected]> 30.08.06 17:40 >>>
> >
> >On Wednesday 30 August 2006 17:28, Jan Beulich wrote:
> >> >Hmm, yes. Sigh, so it's either gcc changes or binutils changes, and
> >> > none of that can be relied upon, except someone has a better idea to
> >> > identify signal frames with high enough assurance to be sensible for
> >> > debugging.
> >>
> >> The only alternative idea I have is to take into consideration the
> >> location of the return address: if it's at the default location (top of
> >> call frame), then assume this is a normal frame, in all other cases
> >> assume it's an interrupt/exception one. This will probably get a few
> >> cases wrong (where the return address is being played with), but it
> >> might be better than the current situation. Andi?
> >
> >Fine by me.
> >
> >At least it will likely be less controversal than changing all the
> > noreturns :)
>
> Here's a patch, including all that I think is necessary once we selectively
> enable (auto-detect) CONFIG_AS_CFI_SIGNAL_FRAME for newer binutils.
> It doesn't include adjustment of the printed address (i.e. for the original
> example, it'd still be kernel_math_error+0 that gets displayed, but the
> unwinder wouldn't get confused anymore.
Thanks.
Isn't a Kconfig patch missing? I don't see any place that defines
CONFIG_AS_CFI_SIGNAL_FRAME. Actually Kconfig wouldn't
be very good for this, so auto testing would be preferable
(like the cfi test is doing)
BTW the tree you generated it against doesn't seem to match the latest
tree. I had to fix some rejects.
Also it would be nice if you could give a full description that could
be used as a commit message.
Other than that it looks good.
Ok maybe a one liner comment on why UNW_DEFAULT_RA does this magic.
-Andi
>Isn't a Kconfig patch missing? I don't see any place that defines
>CONFIG_AS_CFI_SIGNAL_FRAME. Actually Kconfig wouldn't
>be very good for this, so auto testing would be preferable
>(like the cfi test is doing)
Using that framework was the intention (you used a CONFIG_
prefix there, and so did I), but as I wasn't sure about its status,
and as I also was doing this against plain 2.6.18-rc6, I didn't add
the actual detection logic. Actually I also think that should be
done a little differently to allow for better future extension, i.e.
instead of adding to CFLAGS store the auto-detected results in
a header and forcibly -include it.
>BTW the tree you generated it against doesn't seem to match the latest
>tree. I had to fix some rejects.
I didn't create it against your quilt tree, that's true.
>Also it would be nice if you could give a full description that could
>be used as a commit message.
Below.
>Other than that it looks good.
>
>Ok maybe a one liner comment on why UNW_DEFAULT_RA does this magic.
In order to deal with gcc's somewhat broken handling of noreturn
functions (the call instruction in which may be immediately followed
by a subsequent function, thus leading to the call's return address
pointing into that [wrong] function), add heuristics to the unwinder
to distinguish standard call frames from syscall, exception, or
interruption ones. Also provide for utilizing newer gas'
.cfi_signal_frame for non-heuristic based detection, pending
addition of the respective assembler feature detection logic.
Jan
On Tuesday 12 September 2006 08:57, Jan Beulich wrote:
> >Isn't a Kconfig patch missing? I don't see any place that defines
> >CONFIG_AS_CFI_SIGNAL_FRAME. Actually Kconfig wouldn't
> >be very good for this, so auto testing would be preferable
> >(like the cfi test is doing)
>
> Using that framework was the intention (you used a CONFIG_
> prefix there, and so did I), but as I wasn't sure about its status,
> and as I also was doing this against plain 2.6.18-rc6, I didn't add
> the actual detection logic. Actually I also think that should be
> done a little differently to allow for better future extension, i.e.
> instead of adding to CFLAGS store the auto-detected results in
> a header and forcibly -include it.
Ok. I guess I'll do it in the same way as the CFI detection
and maybe one of the kbuild folks can figure out a better way longer term.
BTW which binutils release started supporting this properly?
-Andi
On Tue, Sep 12, 2006 at 08:32:06AM +0200, Andi Kleen wrote:
> On Tuesday 12 September 2006 08:57, Jan Beulich wrote:
> > >Isn't a Kconfig patch missing? I don't see any place that defines
> > >CONFIG_AS_CFI_SIGNAL_FRAME. Actually Kconfig wouldn't
> > >be very good for this, so auto testing would be preferable
> > >(like the cfi test is doing)
> >
> > Using that framework was the intention (you used a CONFIG_
> > prefix there, and so did I), but as I wasn't sure about its status,
> > and as I also was doing this against plain 2.6.18-rc6, I didn't add
> > the actual detection logic. Actually I also think that should be
> > done a little differently to allow for better future extension, i.e.
> > instead of adding to CFLAGS store the auto-detected results in
> > a header and forcibly -include it.
>
> Ok. I guess I'll do it in the same way as the CFI detection
> and maybe one of the kbuild folks can figure out a better way longer term.
>
> BTW which binutils release started supporting this properly?
2006-02-27 and later CVS, so in H.J's releases that's 2.16.91.0.7 and later
(and in upstream 2.17 and later). But there are several distributions that
backported the changes even to older binutils, so doing a compile time check
if assembler groks .cfi_signal_frame is preferrable to a version check.
Jakub
>BTW which binutils release started supporting this properly?
2.17
Jan