[v2] stack: guess endian for stack frame walking

Message ID 20180921043417.9261-1-npiggin@gmail.com
State Superseded
Headers show
Series
  • [v2] stack: guess endian for stack frame walking
Related show

Checks

Context Check Description
snowpatch_ozlabs/build-multiarch fail Test build-multiarch on branch master
snowpatch_ozlabs/apply_patch success master/apply_patch Successfully applied

Commit Message

Nicholas Piggin Sept. 21, 2018, 4:34 a.m.
The stack unwinder currently does not do any endian conversion, which
means it won't work correctly if the stack does not match pdbg endian.

This patch attempts an endian flip if the stack looks wrong, and goes
with that if it's an improvement. This was nice for debugging skiboot.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
v2: Improve ugly code slightly

 src/thread.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 5 deletions(-)

Comments

Alistair Popple Sept. 21, 2018, 5:17 a.m. | #1
Nice! Must admit I haven't read the code in depth but what condition does the
haphazard endian detection use to detemine endianess?

- Alistair

On Friday, 21 September 2018 2:34:17 PM AEST Nicholas Piggin wrote:
> The stack unwinder currently does not do any endian conversion, which
> means it won't work correctly if the stack does not match pdbg endian.
> 
> This patch attempts an endian flip if the stack looks wrong, and goes
> with that if it's an improvement. This was nice for debugging skiboot.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> v2: Improve ugly code slightly
> 
>  src/thread.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/src/thread.c b/src/thread.c
> index d282307..b53dccd 100644
> --- a/src/thread.c
> +++ b/src/thread.c
> @@ -105,6 +105,15 @@ static int load8(struct pdbg_target *target, uint64_t addr, uint64_t *value)
>  	return 1;
>  }
>  
> +uint64_t flip_endian(uint64_t v)
> +{
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +	return be64toh(v);
> +#else
> +	return le64toh(v);
> +#endif
> +}
> +
>  static int dump_stack(struct thread_regs *regs)
>  {
>  	struct pdbg_target *target;
> @@ -117,24 +126,65 @@ static int dump_stack(struct thread_regs *regs)
>  		break;
>  	}
>  
> -	printf("STACK:\n");
> +	printf("STACK:           SP                NIA\n");
>  	if (!target)
>  		pdbg_log(PDBG_ERROR, "Unable to read memory (no ADU found)\n");
>  
>  	if (sp && is_real_address(regs, sp)) {
> -		if (!load8(target, sp, &sp))
> +		uint64_t tmp;
> +		bool flip = false;
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +		bool be = false;
> +#else
> +		bool be = true;
> +#endif
> +
> +		if (!load8(target, sp, &tmp))
>  			return 1;
> -		while (sp && is_real_address(regs, sp)) {
> +
> +		if (!tmp) {
> +badstack:
> +			printf("SP:0x%016" PRIx64 " points to 0x%016" PRIx64 ", not decoding\n", sp, tmp);
> +			return 0;
> +		}
> +
> +		/* Haphazard endian detection */
> +		if (tmp < sp || (tmp - sp > (1024*1024*1024))) {
> +			uint64_t tmp2;
> +			tmp2 = flip_endian(tmp);
> +			if (tmp2 < sp || (tmp2 - sp > (1024*1024*1024)))
> +				goto badstack;
> +			sp = tmp2;
> +			flip = true;
> +			be = !be;
> +		} else {
> +			sp = tmp;
> +		}
> +
> +		if (be)
> +			printf("Looks like big-endian\n");
> +		else
> +			printf("Looks like little-endian\n");
> +
> +		while (sp) {
> +			if (!is_real_address(regs, sp))
> +				break;
> +
>  			if (!load8(target, sp + 16, &pc))
>  				return 1;
> +			if (flip)
> +				pc = flip_endian(pc);
>  
> -			printf(" 0x%016" PRIx64 " 0x%16" PRIx64 "\n", sp, pc);
> +			printf(" 0x%016" PRIx64 " 0x%016" PRIx64 "\n", sp, pc);
>  
>  			if (!load8(target, sp, &sp))
>  				return 1;
> +			if (flip)
> +				sp = flip_endian(sp);
>  		}
> +	} else {
> +		printf("SP:0x%016" PRIx64 " does not appear to be a stack\n", sp);
>  	}
> -
>  	return 0;
>  }
>  
>
Nicholas Piggin Sept. 21, 2018, 6:03 a.m. | #2
On Fri, 21 Sep 2018 15:17:45 +1000
Alistair Popple <alistair@popple.id.au> wrote:

> Nice! Must admit I haven't read the code in depth but what condition does the
> haphazard endian detection use to detemine endianess?

Mainly it's comparing r1 with the saved stack pointer in r1's frame.
If they are more than 1GB apart, try flipping and see if they're
close.

Another check (which may not catch endian bugs but helps verify we
have a stack) is that stack grows down, so if what we think is the
saved pointer is less than current pointer (we're walking backwards),
then it's also not right.

Thanks,
Nick

> 
> - Alistair
> 
> On Friday, 21 September 2018 2:34:17 PM AEST Nicholas Piggin wrote:
> > The stack unwinder currently does not do any endian conversion, which
> > means it won't work correctly if the stack does not match pdbg endian.
> > 
> > This patch attempts an endian flip if the stack looks wrong, and goes
> > with that if it's an improvement. This was nice for debugging skiboot.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > v2: Improve ugly code slightly
> > 
> >  src/thread.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 55 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/thread.c b/src/thread.c
> > index d282307..b53dccd 100644
> > --- a/src/thread.c
> > +++ b/src/thread.c
> > @@ -105,6 +105,15 @@ static int load8(struct pdbg_target *target, uint64_t addr, uint64_t *value)
> >  	return 1;
> >  }
> >  
> > +uint64_t flip_endian(uint64_t v)
> > +{
> > +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> > +	return be64toh(v);
> > +#else
> > +	return le64toh(v);
> > +#endif
> > +}
> > +
> >  static int dump_stack(struct thread_regs *regs)
> >  {
> >  	struct pdbg_target *target;
> > @@ -117,24 +126,65 @@ static int dump_stack(struct thread_regs *regs)
> >  		break;
> >  	}
> >  
> > -	printf("STACK:\n");
> > +	printf("STACK:           SP                NIA\n");
> >  	if (!target)
> >  		pdbg_log(PDBG_ERROR, "Unable to read memory (no ADU found)\n");
> >  
> >  	if (sp && is_real_address(regs, sp)) {
> > -		if (!load8(target, sp, &sp))
> > +		uint64_t tmp;
> > +		bool flip = false;
> > +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> > +		bool be = false;
> > +#else
> > +		bool be = true;
> > +#endif
> > +
> > +		if (!load8(target, sp, &tmp))
> >  			return 1;
> > -		while (sp && is_real_address(regs, sp)) {
> > +
> > +		if (!tmp) {
> > +badstack:
> > +			printf("SP:0x%016" PRIx64 " points to 0x%016" PRIx64 ", not decoding\n", sp, tmp);
> > +			return 0;
> > +		}
> > +
> > +		/* Haphazard endian detection */
> > +		if (tmp < sp || (tmp - sp > (1024*1024*1024))) {
> > +			uint64_t tmp2;
> > +			tmp2 = flip_endian(tmp);
> > +			if (tmp2 < sp || (tmp2 - sp > (1024*1024*1024)))
> > +				goto badstack;
> > +			sp = tmp2;
> > +			flip = true;
> > +			be = !be;
> > +		} else {
> > +			sp = tmp;
> > +		}
> > +
> > +		if (be)
> > +			printf("Looks like big-endian\n");
> > +		else
> > +			printf("Looks like little-endian\n");
> > +
> > +		while (sp) {
> > +			if (!is_real_address(regs, sp))
> > +				break;
> > +
> >  			if (!load8(target, sp + 16, &pc))
> >  				return 1;
> > +			if (flip)
> > +				pc = flip_endian(pc);
> >  
> > -			printf(" 0x%016" PRIx64 " 0x%16" PRIx64 "\n", sp, pc);
> > +			printf(" 0x%016" PRIx64 " 0x%016" PRIx64 "\n", sp, pc);
> >  
> >  			if (!load8(target, sp, &sp))
> >  				return 1;
> > +			if (flip)
> > +				sp = flip_endian(sp);
> >  		}
> > +	} else {
> > +		printf("SP:0x%016" PRIx64 " does not appear to be a stack\n", sp);
> >  	}
> > -
> >  	return 0;
> >  }
> >  
> >   
> 
>

Patch

diff --git a/src/thread.c b/src/thread.c
index d282307..b53dccd 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -105,6 +105,15 @@  static int load8(struct pdbg_target *target, uint64_t addr, uint64_t *value)
 	return 1;
 }
 
+uint64_t flip_endian(uint64_t v)
+{
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+	return be64toh(v);
+#else
+	return le64toh(v);
+#endif
+}
+
 static int dump_stack(struct thread_regs *regs)
 {
 	struct pdbg_target *target;
@@ -117,24 +126,65 @@  static int dump_stack(struct thread_regs *regs)
 		break;
 	}
 
-	printf("STACK:\n");
+	printf("STACK:           SP                NIA\n");
 	if (!target)
 		pdbg_log(PDBG_ERROR, "Unable to read memory (no ADU found)\n");
 
 	if (sp && is_real_address(regs, sp)) {
-		if (!load8(target, sp, &sp))
+		uint64_t tmp;
+		bool flip = false;
+#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
+		bool be = false;
+#else
+		bool be = true;
+#endif
+
+		if (!load8(target, sp, &tmp))
 			return 1;
-		while (sp && is_real_address(regs, sp)) {
+
+		if (!tmp) {
+badstack:
+			printf("SP:0x%016" PRIx64 " points to 0x%016" PRIx64 ", not decoding\n", sp, tmp);
+			return 0;
+		}
+
+		/* Haphazard endian detection */
+		if (tmp < sp || (tmp - sp > (1024*1024*1024))) {
+			uint64_t tmp2;
+			tmp2 = flip_endian(tmp);
+			if (tmp2 < sp || (tmp2 - sp > (1024*1024*1024)))
+				goto badstack;
+			sp = tmp2;
+			flip = true;
+			be = !be;
+		} else {
+			sp = tmp;
+		}
+
+		if (be)
+			printf("Looks like big-endian\n");
+		else
+			printf("Looks like little-endian\n");
+
+		while (sp) {
+			if (!is_real_address(regs, sp))
+				break;
+
 			if (!load8(target, sp + 16, &pc))
 				return 1;
+			if (flip)
+				pc = flip_endian(pc);
 
-			printf(" 0x%016" PRIx64 " 0x%16" PRIx64 "\n", sp, pc);
+			printf(" 0x%016" PRIx64 " 0x%016" PRIx64 "\n", sp, pc);
 
 			if (!load8(target, sp, &sp))
 				return 1;
+			if (flip)
+				sp = flip_endian(sp);
 		}
+	} else {
+		printf("SP:0x%016" PRIx64 " does not appear to be a stack\n", sp);
 	}
-
 	return 0;
 }