[edk2,1/2] ArmPkg/DefaultExceptionHandlerLib: use deadloop rather than ASSERT

Message ID 1462804415-4007-1-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit 1549fb607b50c814072d18229ca423acb150cb68
Headers show

Commit Message

Ard Biesheuvel May 9, 2016, 2:33 p.m.
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

Comments

Laszlo Ersek May 9, 2016, 3:11 p.m. | #1
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
Ard Biesheuvel May 9, 2016, 3:15 p.m. | #2
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
Leif Lindholm May 9, 2016, 3:18 p.m. | #3
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

Patch

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 ();
 }