Re: [PATCH v3 3/3] MdePkg: add NORETURN attribute to LongJump and InternalLongJump

[Michael Zimmermann <sigmaepsilon92@gmail.com>]

Liming:
It's not required to fix the bug which generates bad code. The NORETURN
attribute just fixes the compiler warning mentioned in the commit message
so I'm fine with separating this patch from the series.

And actually this "unreachable code" warning sounds like a more complicated
problem because while the compiler is right and the code after the call to
LongJump should never run there is a slight possibility for that to happen
anyway - just imagine that because of memory corruption the PC/LR inside
the jump buffer point to after the call of longjump.

On Mon, Jan 8, 2018 at 6:34 AM, Gao, Liming <liming.gao@intel.com> wrote:

> Michael:
>   After more test, this patch will cause MdeModulePkg DxeCore link failure
> on VS2015x86 tool chain. The warning message is caused by NORETURN
> attribute in LongJump() API. Is this patch really required? Could we
> separate it from the change RETURNS_TWICE? If yes, I suggest to commit
> RETURNS_TWICE patch first, then work out the solution on NORETURN attribute
> enabling.
>
> c:\edk2\mdemodulepkg\core\dxe\image\image.c(1863) : error C2220: warning
> treated as error - no 'executable' file generated
> c:\edk2\mdemodulepkg\core\dxe\image\image.c(1863) : warning C4702:
> unreachable code
> c:\edk2\mdemodulepkg\core\dxe\image\image.c(1864) : warning C4702:
> unreachable code
>
> >-----Original Message-----
> >From: M1cha [mailto:sigmaepsilon92@gmail.com]
> >Sent: Thursday, December 28, 2017 3:29 AM
> >To: edk2-devel@lists.01.org
> >Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, Michael D
> ><michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> >Subject: [edk2] [PATCH v3 3/3] MdePkg: add NORETURN attribute to
> >LongJump and InternalLongJump
> >
> >This fixes compiler warnings when using them in functions which
> >should return a value but rely on LongJump to never return instead.
> >
> >Contributed-under: TianoCore Contribution Agreement 1.1
> >Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
> >---
> >V3: fix VS compilation errors
> >
> > MdePkg/Include/Library/BaseLib.h             | 1 +
> > MdePkg/Library/BaseLib/BaseLibInternals.h    | 1 +
> > MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c | 1 +
> > MdePkg/Library/BaseLib/Ia32/LongJump.c       | 1 +
> > MdePkg/Library/BaseLib/LongJump.c            | 1 +
> > 5 files changed, 5 insertions(+)
> >
> >diff --git a/MdePkg/Include/Library/BaseLib.h
> >b/MdePkg/Include/Library/BaseLib.h
> >index 10976032adaa..c6783f4624e2 100644
> >--- a/MdePkg/Include/Library/BaseLib.h
> >+++ b/MdePkg/Include/Library/BaseLib.h
> >@@ -4927,6 +4927,7 @@ SetJump (
> >                       restored and must be non-zero.
> >
> > **/
> >+NORETURN
> > VOID
> > EFIAPI
> > LongJump (
> >diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h
> >b/MdePkg/Library/BaseLib/BaseLibInternals.h
> >index 9dca97a0dcc9..5cc83d2956e5 100644
> >--- a/MdePkg/Library/BaseLib/BaseLibInternals.h
> >+++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
> >@@ -441,6 +441,7 @@ InternalAssertJumpBuffer (
> >   @param  Value         The value to return when the SetJump() context is
> >restored.
> >
> > **/
> >+NORETURN
> > VOID
> > EFIAPI
> > InternalLongJump (
> >diff --git a/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
> >b/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
> >index e309e8b57d7a..f48d7d17c4e4 100644
> >--- a/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
> >+++ b/MdePkg/Library/BaseLib/Ebc/SetJumpLongJump.c
> >@@ -54,6 +54,7 @@ SetJump (
> >   @param  Value         The value to return when the SetJump() context is
> >restored.
> >
> > **/
> >+NORETURN
> > VOID
> > EFIAPI
> > InternalLongJump (
> >diff --git a/MdePkg/Library/BaseLib/Ia32/LongJump.c
> >b/MdePkg/Library/BaseLib/Ia32/LongJump.c
> >index 73973a9cceae..8fc86f9efb69 100644
> >--- a/MdePkg/Library/BaseLib/Ia32/LongJump.c
> >+++ b/MdePkg/Library/BaseLib/Ia32/LongJump.c
> >@@ -28,6 +28,7 @@
> >
> > **/
> > __declspec (naked)
> >+NORETURN
> > VOID
> > EFIAPI
> > InternalLongJump (
> >diff --git a/MdePkg/Library/BaseLib/LongJump.c
> >b/MdePkg/Library/BaseLib/LongJump.c
> >index 062be8f2daa1..fef764d6787e 100644
> >--- a/MdePkg/Library/BaseLib/LongJump.c
> >+++ b/MdePkg/Library/BaseLib/LongJump.c
> >@@ -33,6 +33,7 @@
> >                       restored and must be non-zero.
> >
> > **/
> >+NORETURN
> > VOID
> > EFIAPI
> > LongJump (
> >--
> >2.15.1
>
>