| Message ID | 20181211132510.21359-1-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [edk2] ArmPkg/DefaultExceptionHandlerLib ARM: avoid endless loop in RELEASE builds | expand |
On Tue, Dec 11, 2018 at 02:25:10PM +0100, Ard Biesheuvel wrote: > Ensure that we prevent the CPU from proceeding after having taken an > unhandled exception on a RELEASE build, which does not contain the > ASSERT() which ensures this on DEBUG and NOOPT builds. Sounds like a good idea. Some silly questions below: > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c > index 0b9da031b47d..9d96d5aabd96 100644 > --- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c > +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c > @@ -267,10 +267,15 @@ DefaultExceptionHandler ( > DEBUG ((EFI_D_ERROR, "\n")); > ASSERT (FALSE); > > + if (!PcAdjust) { Won't this always be 0 for a RELEASE build? (By convention if nothing else.) And won't we already have ASSERTed in any other case? > + CpuDeadLoop (); > + } > + > // Clear the error registers that we have already displayed incase some one wants to keep going > SystemContext.SystemContextArm->DFSR = 0; > SystemContext.SystemContextArm->IFSR = 0; > > // If some one is stepping past the exception handler adjust the PC to point to the next instruction > SystemContext.SystemContextArm->PC += PcAdjust; > + Hmm? / Leif > } > -- > 2.19.2 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Tue, 11 Dec 2018 at 15:03, Leif Lindholm <leif.lindholm@linaro.org> wrote: > > On Tue, Dec 11, 2018 at 02:25:10PM +0100, Ard Biesheuvel wrote: > > Ensure that we prevent the CPU from proceeding after having taken an > > unhandled exception on a RELEASE build, which does not contain the > > ASSERT() which ensures this on DEBUG and NOOPT builds. > > Sounds like a good idea. > Some silly questions below: > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- > > ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c > > index 0b9da031b47d..9d96d5aabd96 100644 > > --- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c > > +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c > > @@ -267,10 +267,15 @@ DefaultExceptionHandler ( > > DEBUG ((EFI_D_ERROR, "\n")); > > ASSERT (FALSE); > > > > + if (!PcAdjust) { > > Won't this always be 0 for a RELEASE build? (By convention if nothing else.) > And won't we already have ASSERTed in any other case? > Yeah. I had it in my mind that you could set PcAdjust from the debugger, but that doesn't really fly given that the optimizer will just get rid of it in a RELEASE build, and you'd hit the ASSERT() otherwise. > > + CpuDeadLoop (); > > + } > > + > > // Clear the error registers that we have already displayed incase some one wants to keep going > > SystemContext.SystemContextArm->DFSR = 0; > > SystemContext.SystemContextArm->IFSR = 0; > > > > // If some one is stepping past the exception handler adjust the PC to point to the next instruction > > SystemContext.SystemContextArm->PC += PcAdjust; > > + > > Hmm? > Oops. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c index 0b9da031b47d..9d96d5aabd96 100644 --- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c @@ -267,10 +267,15 @@ DefaultExceptionHandler ( DEBUG ((EFI_D_ERROR, "\n")); ASSERT (FALSE); + if (!PcAdjust) { + CpuDeadLoop (); + } + // Clear the error registers that we have already displayed incase some one wants to keep going SystemContext.SystemContextArm->DFSR = 0; SystemContext.SystemContextArm->IFSR = 0; // If some one is stepping past the exception handler adjust the PC to point to the next instruction SystemContext.SystemContextArm->PC += PcAdjust; + }
Ensure that we prevent the CPU from proceeding after having taken an unhandled exception on a RELEASE build, which does not contain the ASSERT() which ensures this on DEBUG and NOOPT builds. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 5 +++++ 1 file changed, 5 insertions(+) -- 2.19.2 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel