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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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


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