2.6.5 unwind problem with rp <- r0

2.6.5 unwind problem with rp <- r0

[Keith Owens]

Unwinding through a kernel thread, we hit kernel_thread_helper which is
called from start_kernel_thread with unwind data

        .save rp, r0                            // this is the end of the call-chain

The kernel unwinder does not handle this correctly, it spits
  unwind.pt_regs_off: bad scratch reg r0
and keeps unwinding, generating garbage.

Re: 2.6.5 unwind problem with rp <- r0

[David Mosberger]

>>>>> On Thu, 29 Apr 2004 14:08:31 +1000, Keith Owens <kaos@sgi.com> said:

  Keith> Unwinding through a kernel thread, we hit
  Keith> kernel_thread_helper which is called from start_kernel_thread
  Keith> with unwind data

  Keith>         .save rp, r0 // this is the end of the call-chain

  Keith> The kernel unwinder does not handle this correctly, it spits
  Keith> unwind.pt_regs_off: bad scratch reg r0 and keeps unwinding,
  Keith> generating garbage.

For now, I think the best solution is to avoid the idiom.  I don't
want to make too many changes to the existing kernel unwinder anymore.
That is, I'd rather spend time on a libunwind-based kernel unwinder.

	--david

Re: 2.6.5 unwind problem with rp <- r0

[Keith Owens]

On Mon, 3 May 2004 15:28:18 -0700, 
David Mosberger <davidm@napali.hpl.hp.com> wrote:
>>>>>> On Thu, 29 Apr 2004 14:08:31 +1000, Keith Owens <kaos@sgi.com> said:
>
>  Keith> Unwinding through a kernel thread, we hit
>  Keith> kernel_thread_helper which is called from start_kernel_thread
>  Keith> with unwind data
>
>  Keith>         .save rp, r0 // this is the end of the call-chain
>
>  Keith> The kernel unwinder does not handle this correctly, it spits
>  Keith> unwind.pt_regs_off: bad scratch reg r0 and keeps unwinding,
>  Keith> generating garbage.
>
>For now, I think the best solution is to avoid the idiom.  I don't
>want to make too many changes to the existing kernel unwinder anymore.
>That is, I'd rather spend time on a libunwind-based kernel unwinder.

David, you wanted rp <- r0 to get a clean termination of the unwind
chain.  Without that clean termination, unwind wanders off into nowhere
generating garbage.  This has already confused at least two people.

Re: 2.6.5 unwind problem with rp <- r0

[David Mosberger]

>>>>> On Tue, 04 May 2004 12:07:25 +1000, Keith Owens <kaos@sgi.com> said:

  Keith> David, you wanted rp <- r0 to get a clean termination of the
  Keith> unwind chain.  Without that clean termination, unwind wanders
  Keith> off into nowhere generating garbage.  This has already
  Keith> confused at least two people.

Did you not read the changelog entry where I explained why I chose
this approach?

	--david

Re: 2.6.5 unwind problem with rp <- r0

[Keith Owens]

On Tue, 4 May 2004 09:40:58 -0700, 
David Mosberger <davidm@napali.hpl.hp.com> wrote:
>>>>>> On Tue, 04 May 2004 12:07:25 +1000, Keith Owens <kaos@sgi.com> said:
>
>  Keith> David, you wanted rp <- r0 to get a clean termination of the
>  Keith> unwind chain.  Without that clean termination, unwind wanders
>  Keith> off into nowhere generating garbage.  This has already
>  Keith> confused at least two people.
>
>Did you not read the changelog entry where I explained why I chose
>this approach?

Which changelog entry?  The only one that comes close is "ia64: Rename
ia64_invoke_kernel_thread_helper() to start_kernel_thread() for
symmetry with start_kernel() and to make it obvious when the end of the
call-chain has been reached".

It is not obvious that the end of the chain has been reached.  The
unwinder keeps going and generates many more useless backtrace entries.

Software Conventions and Runtime Architecture Guide (24535803.pdf)
section 11.1.2 explicitly states "The bottom of the call stack is
identified by a saved return link of 0".

This ugly kludge handles unwinding from a value in r0.  It gets the
kernel unwinder working again, until it can be replaced by libunwind.

Index: linux/arch/ia64/kernel/unwind.c
=================================--- linux.orig/arch/ia64/kernel/unwind.c	Sat May  1 13:15:32 2004
+++ linux/arch/ia64/kernel/unwind.c	Sat May  1 13:15:54 2004
@@ -1417,6 +1417,10 @@
 				need_nat_info = 0;
 			}
 			val = unw.preg_index[UNW_REG_R4 + (rval - 4)];
+		} else if (rval = 0 /*kludge*/) {
+#define KLUDGE_VAL_R0 -1
+			opc = UNW_INSN_MOVE;
+			val = KLUDGE_VAL_R0;
 		} else {
 			/* register got spilled to a scratch register */
 			opc = UNW_INSN_MOVE_SCRATCH;
@@ -1697,6 +1701,7 @@
 	unsigned long opc, dst, val, off;
 	unsigned long *s = (unsigned long *) state;
 	STAT(unsigned long start;)
+	static unsigned long kludge_r0;
 
 	STAT(++unw.stat.script.runs; start = ia64_get_itc());
 	state->flags = script->flags;
@@ -1724,6 +1729,10 @@
 			break;
 
 		      case UNW_INSN_MOVE:
+			if (val = KLUDGE_VAL_R0) {
+				s[dst] = (unsigned long)&kludge_r0;
+				break;
+			}
 			if (!s[val])
 				goto lazy_init;
 			s[dst] = s[val];

Re: 2.6.5 unwind problem with rp <- r0

[David Mosberger]

>>>>> On Wed, 05 May 2004 10:04:58 +1000, Keith Owens <kaos@sgi.com> said:

  Keith> This ugly kludge handles unwinding from a value in r0.  It
  Keith> gets the kernel unwinder working again, until it can be
  Keith> replaced by libunwind.

I hate kludges.

An alternative that might make you happy: in kernel_thread(),
initialize regs.sw.r4 to 0, then change start_kernel_thread's
prologue to:

	.prologue
	.save rp, r4
	.body

Want to try this?

	--david

Re: 2.6.5 unwind problem with rp <- r0

[Keith Owens]

On Wed, 5 May 2004 16:38:25 -0700, 
David Mosberger <davidm@napali.hpl.hp.com> wrote:
>>>>>> On Wed, 05 May 2004 10:04:58 +1000, Keith Owens <kaos@sgi.com> said:
>
>  Keith> This ugly kludge handles unwinding from a value in r0.  It
>  Keith> gets the kernel unwinder working again, until it can be
>  Keith> replaced by libunwind.
>
>I hate kludges.
>
>An alternative that might make you happy: in kernel_thread(),
>initialize regs.sw.r4 to 0, then change start_kernel_thread's
>prologue to:
>
>	.prologue
>	.save rp, r4
>	.body
>
>Want to try this?

I would rather keep the ability to .save rp,r0 and fix the unwinder to
cope, even if it needs a kludge.  After all, a great luminary said that
.save rp,r0 was the correct fix back in March 2003.

http://marc.theaimsgroup.com/?l=linux-ia64&m\x105590709806090&w=2

Re: 2.6.5 unwind problem with rp <- r0

[David Mosberger]

>>>>> On Thu, 06 May 2004 10:08:21 +1000, Keith Owens <kaos@sgi.com> said:

  Keith> I would rather keep the ability to .save rp,r0 and fix the
  Keith> unwinder to cope, even if it needs a kludge.  After all, a
  Keith> great luminary said that .save rp,r0 was the correct fix back
  Keith> in March 2003.

  Keith> http://marc.theaimsgroup.com/?l=linux-ia64&m\x105590709806090&w=2

I'd have preferred if there had been a simple and clean fix to the
existing unwinder that supports .save rp,r0, but I haven't seen/found
something acceptable yet.  Also, the kernel has always been using the
non-race-free version for historical reasons.  In other words: when
the new unwinder goes in, we need to go through the code and replace
".save rp, r4; mov r4=r0" with ".save rp, r0" anyhow.
start_kernel_thread() is special because we can tweak it easily enough
to make the ".save rp, r4" work in a race-free manner.

	--david

Re: 2.6.5 unwind problem with rp <- r0

[Keith Owens]

On Wed, 5 May 2004 17:19:37 -0700, 
David Mosberger <davidm@napali.hpl.hp.com> wrote:
>I'd have preferred if there had been a simple and clean fix to the
>existing unwinder that supports .save rp,r0, but I haven't seen/found
>something acceptable yet.

The clean patch is a bit bigger than the kludge.

Index: 2.6.6-rc3-unwind-r0-fix/arch/ia64/kernel/unwind_i.h
=================================--- 2.6.6-rc3-unwind-r0-fix.orig/arch/ia64/kernel/unwind_i.h	Thu May  6 11:37:18 2004
+++ 2.6.6-rc3-unwind-r0-fix/arch/ia64/kernel/unwind_i.h	Thu May  6 11:37:23 2004
@@ -133,6 +133,7 @@
 	UNW_INSN_SETNAT_TYPE,		/* s[dst+1].nat.type = val */
 	UNW_INSN_LOAD,			/* s[dst] = *s[val] */
 	UNW_INSN_MOVE_SCRATCH,		/* s[dst] = scratch reg "val" */
+	UNW_INSN_MOVE_CONST,		/* s[dst] = constant reg "val" */
 };
 
 struct unw_insn {
Index: 2.6.6-rc3-unwind-r0-fix/arch/ia64/kernel/unwind.c
=================================--- 2.6.6-rc3-unwind-r0-fix.orig/arch/ia64/kernel/unwind.c	Thu May  6 11:37:18 2004
+++ 2.6.6-rc3-unwind-r0-fix/arch/ia64/kernel/unwind.c	Thu May  6 11:57:29 2004
@@ -1407,6 +1407,9 @@
 				need_nat_info = 0;
 			}
 			val = unw.preg_index[UNW_REG_R4 + (rval - 4)];
+		} else if (rval = 0) {
+			opc = UNW_INSN_MOVE_CONST;
+			val = 0;
 		} else {
 			/* register got spilled to a scratch register */
 			opc = UNW_INSN_MOVE_SCRATCH;
@@ -1729,6 +1732,17 @@
 			}
 			break;
 
+		      case UNW_INSN_MOVE_CONST:
+			if (val = 0) {
+				static const unsigned long unw_r0;
+				s[dst] = (unsigned long)&unw_r0;
+			} else {
+				s[dst] = 0;
+				UNW_DPRINT(0, "unwind.%s: UNW_INSN_MOVE_CONST bad val=%ld\n",
+					   __FUNCTION__, val);
+			}
+			break;
+
 		      case UNW_INSN_MOVE_STACKED:
 			s[dst] = (unsigned long) ia64_rse_skip_regs((unsigned long *)state->bsp,
 								    val);

Re: 2.6.5 unwind problem with rp <- r0

[David Mosberger]

>>>>> On Thu, 06 May 2004 12:04:13 +1000, Keith Owens <kaos@sgi.com> said:

  Keith> On Wed, 5 May 2004 17:19:37 -0700, David Mosberger
  Keith> <davidm@napali.hpl.hp.com> wrote:
  >> I'd have preferred if there had been a simple and clean fix to
  >> the existing unwinder that supports .save rp,r0, but I haven't
  >> seen/found something acceptable yet.

  Keith> The clean patch is a bit bigger than the kludge.

I considered that approach but rejected it because it runs the
"FORTRAN constant" risk.  That is, someone might write to a saved
register (e.g., via unw_set_br()) and end up inadvertently redefining
the 0 "constant" (unw_r0) to a non-zero value.

Perhaps what we could do is change unw_access_{gr,br,fr,ar,pr} to
reject attempts to write to the special save-address &unw_r0.  I think
that would make it safe.

	--david

Re: 2.6.5 unwind problem with rp <- r0

[Keith Owens]

On Wed, 5 May 2004 20:42:24 -0700, 
David Mosberger <davidm@napali.hpl.hp.com> wrote:
>I considered that approach but rejected it because it runs the
>"FORTRAN constant" risk.  That is, someone might write to a saved
>register (e.g., via unw_set_br()) and end up inadvertently redefining
>the 0 "constant" (unw_r0) to a non-zero value.
>
>Perhaps what we could do is change unw_access_{gr,br,fr,ar,pr} to
>reject attempts to write to the special save-address &unw_r0.  I think
>that would make it safe.

Unlikely, but if that is what it takes ....

Index: 2.6.6-rc3-unwind-r0-fix/arch/ia64/kernel/unwind.c
=================================--- 2.6.6-rc3-unwind-r0-fix.orig/arch/ia64/kernel/unwind.c	Thu May  6 11:37:18 2004
+++ 2.6.6-rc3-unwind-r0-fix/arch/ia64/kernel/unwind.c	Fri May  7 18:00:49 2004
@@ -239,6 +239,18 @@ static struct {
 #endif
 };
 
+static const unsigned long unw_r0;
+
+static inline int
+unw_write_valid(unsigned long *addr)
+{
+	if (addr = &unw_r0) {
+		UNW_DPRINT(0, "unwind.%s: attempt to write to constant register r0\n", __FUNCTION__);
+		return 0;
+	}
+	return 1;
+}
+
 /* Unwind accessors.  */
 
 /*
@@ -377,11 +389,13 @@ unw_access_gr (struct unw_frame_info *in
 	}
 
 	if (write) {
-		*addr = *val;
-		if (*nat)
-			*nat_addr |= nat_mask;
-		else
-			*nat_addr &= ~nat_mask;
+		if (unw_write_valid(addr)) {
+			*addr = *val;
+			if (*nat)
+				*nat_addr |= nat_mask;
+			else
+				*nat_addr &= ~nat_mask;
+		}
 	} else {
 		if ((*nat_addr & nat_mask) = 0) {
 			*val = *addr;
@@ -419,9 +433,10 @@ unw_access_br (struct unw_frame_info *in
 			   __FUNCTION__, regnum);
 		return -1;
 	}
-	if (write)
-		*addr = *val;
-	else
+	if (write) {
+		if (unw_write_valid(addr))
+			*addr = *val;
+	} else
 		*val = *addr;
 	return 0;
 }
@@ -464,9 +479,10 @@ unw_access_fr (struct unw_frame_info *in
 		addr = t->thread.fph + (regnum - 32);
 	}
 
-	if (write)
-		*addr = *val;
-	else
+	if (write) {
+		if (unw_write_valid((unsigned long *)addr))
+			*addr = *val;
+	} else
 		*val = *addr;
 	return 0;
 }
@@ -557,9 +573,10 @@ unw_access_ar (struct unw_frame_info *in
 		return -1;
 	}
 
-	if (write)
-		*addr = *val;
-	else
+	if (write) {
+		if (unw_write_valid(addr))
+			*addr = *val;
+	} else
 		*val = *addr;
 	return 0;
 }
@@ -574,9 +591,10 @@ unw_access_pr (struct unw_frame_info *in
 	if (!addr)
 		addr = &info->sw->pr;
 
-	if (write)
-		*addr = *val;
-	else
+	if (write) {
+		if (unw_write_valid(addr))
+			*addr = *val;
+	} else
 		*val = *addr;
 	return 0;
 }
@@ -1407,6 +1425,9 @@ compile_reg (struct unw_state_record *sr
 				need_nat_info = 0;
 			}
 			val = unw.preg_index[UNW_REG_R4 + (rval - 4)];
+		} else if (rval = 0) {
+			opc = UNW_INSN_MOVE_CONST;
+			val = 0;
 		} else {
 			/* register got spilled to a scratch register */
 			opc = UNW_INSN_MOVE_SCRATCH;
@@ -1729,6 +1750,16 @@ run_script (struct unw_script *script, s
 			}
 			break;
 
+		      case UNW_INSN_MOVE_CONST:
+			if (val = 0)
+				s[dst] = (unsigned long)&unw_r0;
+			else {
+				s[dst] = 0;
+				UNW_DPRINT(0, "unwind.%s: UNW_INSN_MOVE_CONST bad val=%ld\n",
+					   __FUNCTION__, val);
+			}
+			break;
+
 		      case UNW_INSN_MOVE_STACKED:
 			s[dst] = (unsigned long) ia64_rse_skip_regs((unsigned long *)state->bsp,
 								    val);
Index: 2.6.6-rc3-unwind-r0-fix/arch/ia64/kernel/unwind_i.h
=================================--- 2.6.6-rc3-unwind-r0-fix.orig/arch/ia64/kernel/unwind_i.h	Thu May  6 11:37:18 2004
+++ 2.6.6-rc3-unwind-r0-fix/arch/ia64/kernel/unwind_i.h	Thu May  6 11:37:23 2004
@@ -133,6 +133,7 @@ enum unw_insn_opcode {
 	UNW_INSN_SETNAT_TYPE,		/* s[dst+1].nat.type = val */
 	UNW_INSN_LOAD,			/* s[dst] = *s[val] */
 	UNW_INSN_MOVE_SCRATCH,		/* s[dst] = scratch reg "val" */
+	UNW_INSN_MOVE_CONST,		/* s[dst] = constant reg "val" */
 };
 
 struct unw_insn {

Re: 2.6.5 unwind problem with rp <- r0

[David Mosberger]

>>>>> On Fri, 07 May 2004 18:12:49 +1000, Keith Owens <kaos@sgi.com> said:

  Keith> On Wed, 5 May 2004 20:42:24 -0700, David Mosberger
  Keith> <davidm@napali.hpl.hp.com> wrote:

  >> I considered that approach but rejected it because it runs the
  >> "FORTRAN constant" risk.  That is, someone might write to a saved
  >> register (e.g., via unw_set_br()) and end up inadvertently
  >> redefining the 0 "constant" (unw_r0) to a non-zero value.

  >> Perhaps what we could do is change unw_access_{gr,br,fr,ar,pr} to
  >> reject attempts to write to the special save-address &unw_r0.  I
  >> think that would make it safe.

  Keith> Unlikely, but if that is what it takes ....

Can you try what's in the current bk repository?  It closely follows
your patch, apart from some naming and from switching the asm files to
actually use ".save rp, r0".

Thanks,

	--david

Re: 2.6.5 unwind problem with rp <- r0

[Keith Owens]

On Tue, 11 May 2004 00:03:55 -0700, 
David Mosberger <davidm@napali.hpl.hp.com> wrote:
>>>>>> On Fri, 07 May 2004 18:12:49 +1000, Keith Owens <kaos@sgi.com> said:
>
>  Keith> On Wed, 5 May 2004 20:42:24 -0700, David Mosberger
>  Keith> <davidm@napali.hpl.hp.com> wrote:
>
>  >> I considered that approach but rejected it because it runs the
>  >> "FORTRAN constant" risk.  That is, someone might write to a saved
>  >> register (e.g., via unw_set_br()) and end up inadvertently
>  >> redefining the 0 "constant" (unw_r0) to a non-zero value.
>
>  >> Perhaps what we could do is change unw_access_{gr,br,fr,ar,pr} to
>  >> reject attempts to write to the special save-address &unw_r0.  I
>  >> think that would make it safe.
>
>  Keith> Unlikely, but if that is what it takes ....
>
>Can you try what's in the current bk repository?  It closely follows
>your patch, apart from some naming and from switching the asm files to
>actually use ".save rp, r0".

I don't have bk access.  Where's the patch?

Re: 2.6.5 unwind problem with rp <- r0

[David Mosberger]

>>>>> On Tue, 11 May 2004 21:32:00 +1000, Keith Owens <kaos@sgi.com> said:

  >>  Can you try what's in the current bk repository?  It closely
  >> follows your patch, apart from some naming and from switching the
  >> asm files to actually use ".save rp, r0".

  Keith> I don't have bk access.  Where's the patch?

At:

 http://kernel.org/pub/linux/kernel/ports/ia64/v2.6/testing/cset/index.html

You'll definitely want:

  ChangeSet 2004/05/10 22:48:57-07:00, davidm @ tiger.hpl.hp.com

There is also a tiny fix for unwind.c in:

  ChangeSet 2004/05/10 23:44:48-07:00, davidm @ tiger.hpl.hp.com

but that one isn't strictly needed (it mainly corrects a prototype to
avoid a compiler-warning).

	--david