| Message ID | 20180921043417.9261-1-npiggin@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] stack: guess endian for stack frame walking | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/build-multiarch | fail | Test build-multiarch on branch master |
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; > } > >
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; > > } > > > > > >
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; }
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(-)