| Message ID | 1490043181-20031-3-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 173bf5c847e3ca8b42c11796ce048d8e2e916ff8 |
| Headers | show |
| Series | ArmPkg: increase robustness of the crash handler | expand |
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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)); + } - 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))); + } 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; } } }
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(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel