| Message ID | 1462804415-4007-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 1549fb607b50c814072d18229ca423acb150cb68 |
| Headers | show |
On 05/09/16 16:33, Ard Biesheuvel wrote: > The default exception handler, which is essentially the one that is invoked > for unexpected exceptions, ends with an ASSERT (FALSE), to ensure that > execution halts after dumping the CPU state. However, ASSERTs are compiled > out in RELEASE builds, and since we simply return to wherever the ELR is > pointing, we will not make any progress in case of synchronous aborts, and > the same exception will be taken again immediately, resulting in the string > 'Exception at 0x....' to be printed over and over again. > > So use an explicit deadloop instead. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c > index 37fd57875760..57200ff642c2 100644 > --- a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c > +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c > @@ -181,5 +181,6 @@ DefaultExceptionHandler ( > DEBUG ((EFI_D_ERROR, "\n ESR : EC 0x%02x IL 0x%x ISS 0x%08x\n", (SystemContext.SystemContextAArch64->ESR & 0xFC000000) >> 26, (SystemContext.SystemContextAArch64->ESR >> 25) & 0x1, SystemContext.SystemContextAArch64->ESR & 0x1FFFFFF )); > > DescribeExceptionSyndrome (SystemContext.SystemContextAArch64->ESR); > - ASSERT (FALSE); > + > + CpuDeadLoop (); > } > Funny, I looked at this code just the other day, for <https://bugzilla.redhat.com/show_bug.cgi?id=1333888>. :) Anyway, more to the point. The guidance I got from Mike Kinney for the exact same kind of issue was: add both the assert and the dead loop. This way, in a debug build, the ASSERT() will provide location information (plus behave accordingly to the PCDs that control it), while in a release build, the world will still stop. http://thread.gmane.org/gmane.comp.bios.edk2.devel/3788/focus=4686 Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 9 May 2016 at 17:11, Laszlo Ersek <lersek@redhat.com> wrote: > On 05/09/16 16:33, Ard Biesheuvel wrote: >> The default exception handler, which is essentially the one that is invoked >> for unexpected exceptions, ends with an ASSERT (FALSE), to ensure that >> execution halts after dumping the CPU state. However, ASSERTs are compiled >> out in RELEASE builds, and since we simply return to wherever the ELR is >> pointing, we will not make any progress in case of synchronous aborts, and >> the same exception will be taken again immediately, resulting in the string >> 'Exception at 0x....' to be printed over and over again. >> >> So use an explicit deadloop instead. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c >> index 37fd57875760..57200ff642c2 100644 >> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c >> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c >> @@ -181,5 +181,6 @@ DefaultExceptionHandler ( >> DEBUG ((EFI_D_ERROR, "\n ESR : EC 0x%02x IL 0x%x ISS 0x%08x\n", (SystemContext.SystemContextAArch64->ESR & 0xFC000000) >> 26, (SystemContext.SystemContextAArch64->ESR >> 25) & 0x1, SystemContext.SystemContextAArch64->ESR & 0x1FFFFFF )); >> >> DescribeExceptionSyndrome (SystemContext.SystemContextAArch64->ESR); >> - ASSERT (FALSE); >> + >> + CpuDeadLoop (); >> } >> > > Funny, I looked at this code just the other day, for > <https://bugzilla.redhat.com/show_bug.cgi?id=1333888>. :) > I feel for you, brother :-) > Anyway, more to the point. The guidance I got from Mike Kinney for the > exact same kind of issue was: add both the assert and the dead loop. > This way, in a debug build, the ASSERT() will provide location > information (plus behave accordingly to the PCDs that control it), while > in a release build, the world will still stop. > > http://thread.gmane.org/gmane.comp.bios.edk2.devel/3788/focus=4686 > That sounds reasonable, indeed. I will add back the ASSERT when applying (as soon as Leif has had the chance to have his say about this) Thanks, Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Mon, May 09, 2016 at 05:15:11PM +0200, Ard Biesheuvel wrote: > I feel for you, brother :-) > > > Anyway, more to the point. The guidance I got from Mike Kinney for the > > exact same kind of issue was: add both the assert and the dead loop. > > This way, in a debug build, the ASSERT() will provide location > > information (plus behave accordingly to the PCDs that control it), while > > in a release build, the world will still stop. > > > > http://thread.gmane.org/gmane.comp.bios.edk2.devel/3788/focus=4686 > > That sounds reasonable, indeed. I will add back the ASSERT when > applying (as soon as Leif has had the chance to have his say about > this) I'm happy for this (ASSERT + deadloop) and the added stackdump (2/2) to go in. / Leif _______________________________________________ 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 37fd57875760..57200ff642c2 100644 --- a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c @@ -181,5 +181,6 @@ DefaultExceptionHandler ( DEBUG ((EFI_D_ERROR, "\n ESR : EC 0x%02x IL 0x%x ISS 0x%08x\n", (SystemContext.SystemContextAArch64->ESR & 0xFC000000) >> 26, (SystemContext.SystemContextAArch64->ESR >> 25) & 0x1, SystemContext.SystemContextAArch64->ESR & 0x1FFFFFF )); DescribeExceptionSyndrome (SystemContext.SystemContextAArch64->ESR); - ASSERT (FALSE); + + CpuDeadLoop (); }
The default exception handler, which is essentially the one that is invoked for unexpected exceptions, ends with an ASSERT (FALSE), to ensure that execution halts after dumping the CPU state. However, ASSERTs are compiled out in RELEASE builds, and since we simply return to wherever the ELR is pointing, we will not make any progress in case of synchronous aborts, and the same exception will be taken again immediately, resulting in the string 'Exception at 0x....' to be printed over and over again. So use an explicit deadloop instead. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) -- 2.7.4 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel