* [PATCH v3 1/3] MdePkg: add RETURNS_TWICE attribute @ 2017-12-27 19:28 M1cha 2017-12-27 19:28 ` [PATCH v3 3/3] MdePkg: add NORETURN attribute to LongJump and InternalLongJump M1cha 0 siblings, 1 reply; 5+ messages in thread From: M1cha @ 2017-12-27 19:28 UTC (permalink / raw) To: edk2-devel; +Cc: Ard Biesheuvel, Michael D Kinney, Liming Gao Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com> --- V3: add code comments MdePkg/Include/Base.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h index 22ab5d3715fb..8c7345609ed5 100644 --- a/MdePkg/Include/Base.h +++ b/MdePkg/Include/Base.h @@ -218,6 +218,26 @@ VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4); #endif #endif +/// +/// Tell the code optimizer that the function will return twice. +/// This prevents wrong optimizations which can cause bugs. +/// +#ifndef RETURNS_TWICE + #if defined (__GNUC__) || defined (__clang__) + /// + /// Tell the code optimizer that the function will return twice. + /// This prevents wrong optimizations which can cause bugs. + /// + #define RETURNS_TWICE __attribute__((returns_twice)) + #else + /// + /// Tell the code optimizer that the function will return twice. + /// This prevents wrong optimizations which can cause bugs. + /// + #define RETURNS_TWICE + #endif +#endif + // // For symbol name in assembly code, an extra "_" is sometimes necessary // -- 2.15.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v3 3/3] MdePkg: add NORETURN attribute to LongJump and InternalLongJump 2017-12-27 19:28 [PATCH v3 1/3] MdePkg: add RETURNS_TWICE attribute M1cha @ 2017-12-27 19:28 ` M1cha 2018-01-08 5:34 ` Gao, Liming 0 siblings, 1 reply; 5+ messages in thread From: M1cha @ 2017-12-27 19:28 UTC (permalink / raw) To: edk2-devel; +Cc: Ard Biesheuvel, Michael D Kinney, Liming Gao 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3 3/3] MdePkg: add NORETURN attribute to LongJump and InternalLongJump 2017-12-27 19:28 ` [PATCH v3 3/3] MdePkg: add NORETURN attribute to LongJump and InternalLongJump M1cha @ 2018-01-08 5:34 ` Gao, Liming 2018-01-08 6:22 ` Michael Zimmermann 0 siblings, 1 reply; 5+ messages in thread From: Gao, Liming @ 2018-01-08 5:34 UTC (permalink / raw) To: M1cha, edk2-devel@lists.01.org; +Cc: Ard Biesheuvel, Kinney, Michael D 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 3/3] MdePkg: add NORETURN attribute to LongJump and InternalLongJump 2018-01-08 5:34 ` Gao, Liming @ 2018-01-08 6:22 ` Michael Zimmermann 2018-01-10 3:45 ` Gao, Liming 0 siblings, 1 reply; 5+ messages in thread From: Michael Zimmermann @ 2018-01-08 6:22 UTC (permalink / raw) To: Gao, Liming; +Cc: edk2-devel@lists.01.org, Ard Biesheuvel, Kinney, Michael D 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3 3/3] MdePkg: add NORETURN attribute to LongJump and InternalLongJump 2018-01-08 6:22 ` Michael Zimmermann @ 2018-01-10 3:45 ` Gao, Liming 0 siblings, 0 replies; 5+ messages in thread From: Gao, Liming @ 2018-01-10 3:45 UTC (permalink / raw) To: Michael Zimmermann Cc: Kinney, Michael D, edk2-devel@lists.01.org, Ard Biesheuvel Michael: Just push the change on RETURNS_TWICE > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Michael Zimmermann > Sent: Monday, January 8, 2018 2:22 PM > To: Gao, Liming <liming.gao@intel.com> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org> > Subject: Re: [edk2] [PATCH v3 3/3] MdePkg: add NORETURN attribute to LongJump and InternalLongJump > > 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 > > > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-01-10 3:40 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-12-27 19:28 [PATCH v3 1/3] MdePkg: add RETURNS_TWICE attribute M1cha 2017-12-27 19:28 ` [PATCH v3 3/3] MdePkg: add NORETURN attribute to LongJump and InternalLongJump M1cha 2018-01-08 5:34 ` Gao, Liming 2018-01-08 6:22 ` Michael Zimmermann 2018-01-10 3:45 ` Gao, Liming
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox