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

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

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?

> +        } 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>

/
    Leif