Re: [PATCH 2/3] ArmPkg/DefaultExceptionHandlerLib: walk call stack unconditionally

[Leif Lindholm <leif.lindholm@linaro.org>]

On Wed, Mar 22, 2017 at 01:12:59PM +0000, Ard Biesheuvel wrote:
> On 22 March 2017 at 13:07, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Mon, Mar 20, 2017 at 08:53:00PM +0000, Ard Biesheuvel wrote:
> >> Currently, we only attempt to walk the call stack and print a backtrace
> >> if the program counter refers to a location covered by a PE/COFF image.
> >> However, regardless of the value of PC, the frame pointer may still have
> >> a meaningful value, and so we can still produce the remainder of the
> >> backtrace.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 56 +++++++++++---------
> >>  1 file changed, 31 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> >> index 2f9c2ede37c1..1024bf48c63d 100644
> >> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> >> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> >> @@ -181,37 +181,43 @@ DefaultExceptionHandler (
> >>        DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [ 0] %a\n",
> >>          SystemContext.SystemContextAArch64->ELR, ImageBase,
> >>          SystemContext.SystemContextAArch64->ELR - ImageBase, BaseName (Pdb)));
> >> +    } else {
> >> +      DEBUG ((EFI_D_ERROR, "PC 0x%012lx\n", SystemContext.SystemContextAArch64->ELR));
> >> +    }
> >>
> >
> > come_from:
> >
> >> -      if ((UINT64 *)SystemContext.SystemContextAArch64->FP != 0) {
> >> -        Idx = 0;
> >> +    if ((UINT64 *)SystemContext.SystemContextAArch64->FP != 0) {
> >> +      Idx = 0;
> >>
> >> -        RootFp[0] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[0];
> >> -        RootFp[1] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[1];
> >> -        if (RootFp[1] != SystemContext.SystemContextAArch64->LR) {
> >> -          RootFp[0] = SystemContext.SystemContextAArch64->FP;
> >> -          RootFp[1] = SystemContext.SystemContextAArch64->LR;
> >> -        }
> >> -        for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
> >> -          Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
> >> -          if (Pdb != NULL) {
> >> -            if (Pdb != PrevPdb) {
> >> -              Idx++;
> >> -              PrevPdb = Pdb;
> >> -            }
> >> -            DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [% 2d] %a\n",
> >> -              Fp[1], ImageBase, Fp[1] - ImageBase, Idx, BaseName (Pdb)));
> >> +      RootFp[0] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[0];
> >> +      RootFp[1] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[1];
> >> +      if (RootFp[1] != SystemContext.SystemContextAArch64->LR) {
> >> +        RootFp[0] = SystemContext.SystemContextAArch64->FP;
> >> +        RootFp[1] = SystemContext.SystemContextAArch64->LR;
> >> +      }
> >> +      for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
> >> +        Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
> >> +        if (Pdb != NULL) {
> >> +          if (Pdb != PrevPdb) {
> >> +            Idx++;
> >> +            PrevPdb = Pdb;
> >>            }
> >> +          DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [% 2d] %a\n",
> >> +            Fp[1], ImageBase, Fp[1] - ImageBase, Idx, BaseName (Pdb)));
> >
> > Diff's a bit iffy, but can you confirm there is no functional change
> > between come_from and here? Just the indentation shuffle?
> >
> 
> Yes. diff -w output is here http://pastebin.com/gRmeeipp

Thanks - that's a tad more readable :)

> >> +        } else {
> >> +          DEBUG ((EFI_D_ERROR, "PC 0x%012lx\n", Fp[1]));
> >>          }
> >> -        PrevPdb = Pdb = GetImageName (SystemContext.SystemContextAArch64->ELR, &ImageBase, &PeCoffSizeOfHeader);
> >> +      }
> >> +      PrevPdb = Pdb = GetImageName (SystemContext.SystemContextAArch64->ELR, &ImageBase, &PeCoffSizeOfHeader);
> >> +      if (Pdb != NULL) {
> >>          DEBUG ((EFI_D_ERROR, "\n[ 0] %a\n", Pdb));
> >> +      }
> >>
> >> -        Idx = 0;
> >> -        for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
> >> -          Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
> >> -          if (Pdb != NULL && Pdb != PrevPdb) {
> >> -            DEBUG ((EFI_D_ERROR, "[% 2d] %a\n", ++Idx, Pdb));
> >> -            PrevPdb = Pdb;
> >> -          }
> >> +      Idx = 0;
> >> +      for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
> >> +        Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
> >> +        if (Pdb != NULL && Pdb != PrevPdb) {
> >> +          DEBUG ((EFI_D_ERROR, "[% 2d] %a\n", ++Idx, Pdb));
> >> +          PrevPdb = Pdb;
> >>          }
> >>        }
> >>      }
> >> --
> >> 2.7.4
> >>
> >
> > If so:
> > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
> 
> Thanks