[PATCH 0/3] fix GCC optimizations and warnings for SetJump/LongJump

[PATCH 0/3] fix GCC optimizations and warnings for SetJump/LongJump

[M1cha]

I've already discussed this in past but never actually sent proper
patches for some reason.

This patch series is about fixing problems with these functions when
using GCC.

M1cha (3):
  MdePkg: add RETURNS_TWICE attribute
  MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump
  MdePkg: add NORETURN attribute to LongJump and InternalLongJump

 MdePkg/Include/Base.h                     | 10 ++++++++++
 MdePkg/Include/Library/BaseLib.h          |  2 ++
 MdePkg/Library/BaseLib/BaseLibInternals.h |  1 +
 3 files changed, 13 insertions(+)

-- 
2.15.1

[PATCH 1/3] MdePkg: add RETURNS_TWICE attribute

[M1cha]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
---
 MdePkg/Include/Base.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index 22ab5d3715fb..c863de407418 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -218,6 +218,16 @@ VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4);
   #endif
 #endif
 
+#ifndef RETURNS_TWICE
+  #if defined (__GNUC__) || defined (__clang__)
+    #define RETURNS_TWICE  __attribute__((returns_twice))
+  #elif defined(_MSC_EXTENSIONS) && !defined(MDE_CPU_EBC)
+    #define RETURNS_TWICE
+  #else
+    #define RETURNS_TWICE
+  #endif
+#endif
+
 //
 // For symbol name in assembly code, an extra "_" is sometimes necessary
 //
-- 
2.15.1

Re: [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute

[Ard Biesheuvel]

On 22 December 2017 at 07:23, M1cha <sigmaepsilon92@gmail.com> wrote:
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
> ---
>  MdePkg/Include/Base.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
> index 22ab5d3715fb..c863de407418 100644
> --- a/MdePkg/Include/Base.h
> +++ b/MdePkg/Include/Base.h
> @@ -218,6 +218,16 @@ VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4);
>    #endif
>  #endif
>
> +#ifndef RETURNS_TWICE
> +  #if defined (__GNUC__) || defined (__clang__)
> +    #define RETURNS_TWICE  __attribute__((returns_twice))
> +  #elif defined(_MSC_EXTENSIONS) && !defined(MDE_CPU_EBC)
> +    #define RETURNS_TWICE
> +  #else
> +    #define RETURNS_TWICE

What is the point of having two versions that are #defined to nothing?

> +  #endif
> +#endif
> +
>  //
>  // For symbol name in assembly code, an extra "_" is sometimes necessary
>  //
> --
> 2.15.1
>

Re: [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute

[Michael Zimmermann]

The reason for that is that I was lazy when doing copy&paste ;)
I'll send a v2.

On Fri, Dec 22, 2017 at 4:24 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org>
wrote:

> On 22 December 2017 at 07:23, M1cha <sigmaepsilon92@gmail.com> wrote:
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
> > ---
> >  MdePkg/Include/Base.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
> > index 22ab5d3715fb..c863de407418 100644
> > --- a/MdePkg/Include/Base.h
> > +++ b/MdePkg/Include/Base.h
> > @@ -218,6 +218,16 @@ VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE, 4);
> >    #endif
> >  #endif
> >
> > +#ifndef RETURNS_TWICE
> > +  #if defined (__GNUC__) || defined (__clang__)
> > +    #define RETURNS_TWICE  __attribute__((returns_twice))
> > +  #elif defined(_MSC_EXTENSIONS) && !defined(MDE_CPU_EBC)
> > +    #define RETURNS_TWICE
> > +  #else
> > +    #define RETURNS_TWICE
>
> What is the point of having two versions that are #defined to nothing?
>
> > +  #endif
> > +#endif
> > +
> >  //
> >  // For symbol name in assembly code, an extra "_" is sometimes necessary
> >  //
> > --
> > 2.15.1
> >
>

Re: [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute

[Gao, Liming]

Micha:
   Could you add comments for new macro RETURNS_TWICE like others, such as ANALYZER_NORETURN?

>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Friday, December 22, 2017 11:24 PM
>To: M1cha <sigmaepsilon92@gmail.com>
>Cc: edk2-devel@lists.01.org; Kinney, Michael D
><michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
>Subject: Re: [edk2] [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute
>
>On 22 December 2017 at 07:23, M1cha <sigmaepsilon92@gmail.com> wrote:
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
>> ---
>>  MdePkg/Include/Base.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
>> index 22ab5d3715fb..c863de407418 100644
>> --- a/MdePkg/Include/Base.h
>> +++ b/MdePkg/Include/Base.h
>> @@ -218,6 +218,16 @@ VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE,
>4);
>>    #endif
>>  #endif
>>
>> +#ifndef RETURNS_TWICE
>> +  #if defined (__GNUC__) || defined (__clang__)
>> +    #define RETURNS_TWICE  __attribute__((returns_twice))
>> +  #elif defined(_MSC_EXTENSIONS) && !defined(MDE_CPU_EBC)
>> +    #define RETURNS_TWICE
>> +  #else
>> +    #define RETURNS_TWICE
>
>What is the point of having two versions that are #defined to nothing?
>
>> +  #endif
>> +#endif
>> +
>>  //
>>  // For symbol name in assembly code, an extra "_" is sometimes necessary
>>  //
>> --
>> 2.15.1
>>

Re: [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute

[Michael Zimmermann]

Liming:
The other macros have comments both before the compiler directives and
before each define for each compiler.
To me it looks like these are slightly differently formulated only and
kinda redundant too.
Is there a rule or do you have suggestions for writing comments for this
kind of macro?

On Mon, Dec 25, 2017 at 4:11 AM, Gao, Liming <liming.gao@intel.com> wrote:

> Micha:
>    Could you add comments for new macro RETURNS_TWICE like others, such as
> ANALYZER_NORETURN?
>
> >-----Original Message-----
> >From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >Sent: Friday, December 22, 2017 11:24 PM
> >To: M1cha <sigmaepsilon92@gmail.com>
> >Cc: edk2-devel@lists.01.org; Kinney, Michael D
> ><michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> >Subject: Re: [edk2] [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute
> >
> >On 22 December 2017 at 07:23, M1cha <sigmaepsilon92@gmail.com> wrote:
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
> >> ---
> >>  MdePkg/Include/Base.h | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
> >> index 22ab5d3715fb..c863de407418 100644
> >> --- a/MdePkg/Include/Base.h
> >> +++ b/MdePkg/Include/Base.h
> >> @@ -218,6 +218,16 @@ VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE,
> >4);
> >>    #endif
> >>  #endif
> >>
> >> +#ifndef RETURNS_TWICE
> >> +  #if defined (__GNUC__) || defined (__clang__)
> >> +    #define RETURNS_TWICE  __attribute__((returns_twice))
> >> +  #elif defined(_MSC_EXTENSIONS) && !defined(MDE_CPU_EBC)
> >> +    #define RETURNS_TWICE
> >> +  #else
> >> +    #define RETURNS_TWICE
> >
> >What is the point of having two versions that are #defined to nothing?
> >
> >> +  #endif
> >> +#endif
> >> +
> >>  //
> >>  // For symbol name in assembly code, an extra "_" is sometimes
> necessary
> >>  //
> >> --
> >> 2.15.1
> >>
>

Re: [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute

[Gao, Liming]

Michael:
  I suggest to add comments for each definition although there is redundant.

  Besides, after I apply these three patches, and build MdePkg.dsc with VS2015x86. It will report below error. Could you help look it?

        "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Vc\bin\cl.exe" /Foc:\r9tips\allpkg\edk2\Build\Mde\DEBUG_VS2015x86\IA32\MdePkg\Library\BaseCacheMaintenanceLib\BaseCacheMaintenanceLib\OUTPUT\.\X86Cache.obj /nologo /arch:IA32 /c /WX /GS- /W4 /Gs32768 /D UNICODE /O1b2 /GL /FIAutoGen.h /EHs-c- /GR- /GF /Gy /Zi /Gm /Gw -D DISABLE_NEW_DEPRECATED_INTERFACES /Ic:\r9tips\allpkg\edk2\MdePkg\Library\BaseCacheMaintenanceLib  /Ic:\r9tips\allpkg\edk2\Build\Mde\DEBUG_VS2015x86\IA32\MdePkg\Library\BaseCacheMaintenanceLib\BaseCacheMaintenanceLib\DEBUG  /Ic:\r9tips\allpkg\edk2\MdePkg  /Ic:\r9tips\allpkg\edk2\MdePkg\Include  /Ic:\r9tips\allpkg\edk2\MdePkg\Include\Ia32 c:\r9tips\allpkg\edk2\MdePkg\Library\BaseCacheMaintenanceLib\X86Cache.c
X86Cache.c
c:\r9tips\allpkg\edk2\MdePkg\Include\Library/BaseLib.h(4933): error C2059: syntax error: 'type'
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio 14.0\Vc\bin\cl.exe"' : return code '0x2'
Stop.

Thanks
Liming
From: Michael Zimmermann [mailto:sigmaepsilon92@gmail.com]
Sent: Monday, December 25, 2017 11:50 PM
To: Gao, Liming <liming.gao@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2] [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute

Liming:
The other macros have comments both before the compiler directives and before each define for each compiler.
To me it looks like these are slightly differently formulated only and kinda redundant too.
Is there a rule or do you have suggestions for writing comments for this kind of macro?

On Mon, Dec 25, 2017 at 4:11 AM, Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>> wrote:
Micha:
   Could you add comments for new macro RETURNS_TWICE like others, such as ANALYZER_NORETURN?

>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>]
>Sent: Friday, December 22, 2017 11:24 PM
>To: M1cha <sigmaepsilon92@gmail.com<mailto:sigmaepsilon92@gmail.com>>
>Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kinney, Michael D
><michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>
>Subject: Re: [edk2] [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute
>
>On 22 December 2017 at 07:23, M1cha <sigmaepsilon92@gmail.com<mailto:sigmaepsilon92@gmail.com>> wrote:
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com<mailto:sigmaepsilon92@gmail.com>>
>> ---
>>  MdePkg/Include/Base.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
>> index 22ab5d3715fb..c863de407418 100644
>> --- a/MdePkg/Include/Base.h
>> +++ b/MdePkg/Include/Base.h
>> @@ -218,6 +218,16 @@ VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE,
>4);
>>    #endif
>>  #endif
>>
>> +#ifndef RETURNS_TWICE
>> +  #if defined (__GNUC__) || defined (__clang__)
>> +    #define RETURNS_TWICE  __attribute__((returns_twice))
>> +  #elif defined(_MSC_EXTENSIONS) && !defined(MDE_CPU_EBC)
>> +    #define RETURNS_TWICE
>> +  #else
>> +    #define RETURNS_TWICE
>
>What is the point of having two versions that are #defined to nothing?
>
>> +  #endif
>> +#endif
>> +
>>  //
>>  // For symbol name in assembly code, an extra "_" is sometimes necessary
>>  //
>> --
>> 2.15.1
>>

Re: [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute

[Michael Zimmermann]

Liming:
I've never used VS to compile edk2 but to me it looks like it doesn't like
what NORETURN is expanding to ('__declspec(noreturn)').

If that is the case it's a generic bug because this patch set did not add
the NORETURN macro.


On Tue, Dec 26, 2017 at 5:51 PM, Gao, Liming <liming.gao@intel.com> wrote:

> Michael:
>
>   I suggest to add comments for each definition although there is
> redundant.
>
>
>
>   Besides, after I apply these three patches, and build MdePkg.dsc with
> VS2015x86. It will report below error. Could you help look it?
>
>
>
>         "C:\Program Files (x86)\Microsoft Visual Studio
> 14.0\Vc\bin\cl.exe" /Foc:\r9tips\allpkg\edk2\Build\Mde\DEBUG_VS2015x86\
> IA32\MdePkg\Library\BaseCacheMaintenanceLib\BaseCacheMaintenanceLib\OUTPUT\.\X86Cache.obj
> /nologo /arch:IA32 /c /WX /GS- /W4 /Gs32768 /D UNICODE /O1b2 /GL
> /FIAutoGen.h /EHs-c- /GR- /GF /Gy /Zi /Gm /Gw -D DISABLE_NEW_DEPRECATED_INTERFACES
> /Ic:\r9tips\allpkg\edk2\MdePkg\Library\BaseCacheMaintenanceLib
> /Ic:\r9tips\allpkg\edk2\Build\Mde\DEBUG_VS2015x86\IA32\MdePkg\Library\
> BaseCacheMaintenanceLib\BaseCacheMaintenanceLib\DEBUG
> /Ic:\r9tips\allpkg\edk2\MdePkg  /Ic:\r9tips\allpkg\edk2\MdePkg\Include
> /Ic:\r9tips\allpkg\edk2\MdePkg\Include\Ia32 c:\r9tips\allpkg\edk2\MdePkg\
> Library\BaseCacheMaintenanceLib\X86Cache.c
>
> X86Cache.c
>
> c:\r9tips\allpkg\edk2\MdePkg\Include\Library/BaseLib.h(4933): error
> C2059: syntax error: 'type'
>
> NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual
> Studio 14.0\Vc\bin\cl.exe"' : return code '0x2'
>
> Stop.
>
>
>
> Thanks
>
> Liming
>
> *From:* Michael Zimmermann [mailto:sigmaepsilon92@gmail.com]
> *Sent:* Monday, December 25, 2017 11:50 PM
> *To:* Gao, Liming <liming.gao@intel.com>
> *Cc:* Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org;
> Kinney, Michael D <michael.d.kinney@intel.com>
>
> *Subject:* Re: [edk2] [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute
>
>
>
> Liming:
> The other macros have comments both before the compiler directives and
> before each define for each compiler.
>
> To me it looks like these are slightly differently formulated only and
> kinda redundant too.
>
> Is there a rule or do you have suggestions for writing comments for this
> kind of macro?
>
>
>
> On Mon, Dec 25, 2017 at 4:11 AM, Gao, Liming <liming.gao@intel.com> wrote:
>
> Micha:
>    Could you add comments for new macro RETURNS_TWICE like others, such as
> ANALYZER_NORETURN?
>
>
> >-----Original Message-----
> >From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >Sent: Friday, December 22, 2017 11:24 PM
> >To: M1cha <sigmaepsilon92@gmail.com>
> >Cc: edk2-devel@lists.01.org; Kinney, Michael D
> ><michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> >Subject: Re: [edk2] [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute
> >
> >On 22 December 2017 at 07:23, M1cha <sigmaepsilon92@gmail.com> wrote:
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
> >> ---
> >>  MdePkg/Include/Base.h | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
> >> index 22ab5d3715fb..c863de407418 100644
> >> --- a/MdePkg/Include/Base.h
> >> +++ b/MdePkg/Include/Base.h
> >> @@ -218,6 +218,16 @@ VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE,
> >4);
> >>    #endif
> >>  #endif
> >>
> >> +#ifndef RETURNS_TWICE
> >> +  #if defined (__GNUC__) || defined (__clang__)
> >> +    #define RETURNS_TWICE  __attribute__((returns_twice))
> >> +  #elif defined(_MSC_EXTENSIONS) && !defined(MDE_CPU_EBC)
> >> +    #define RETURNS_TWICE
> >> +  #else
> >> +    #define RETURNS_TWICE
> >
> >What is the point of having two versions that are #defined to nothing?
> >
> >> +  #endif
> >> +#endif
> >> +
> >>  //
> >>  // For symbol name in assembly code, an extra "_" is sometimes
> necessary
> >>  //
> >> --
> >> 2.15.1
> >>
>
>
>

Re: [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute

[Gao, Liming]

Michael:
  I try VS compiler. When I add NORETURN as the first attribute of the function declaration. It can pass build. On VS compiler, NORETURN is required to be placed ahead of EFIAPI.

VOID
EFIAPI
NORETURN
LongJump (
  IN      BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer,
  IN      UINTN                     Value
  );

==>

NORETURN
VOID
EFIAPI
LongJump (
  IN      BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer,
  IN      UINTN                     Value
  );

Thanks
Liming
From: Michael Zimmermann [mailto:sigmaepsilon92@gmail.com]
Sent: Wednesday, December 27, 2017 4:01 AM
To: Gao, Liming <liming.gao@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: Re: [edk2] [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute

Liming:
I've never used VS to compile edk2 but to me it looks like it doesn't like what NORETURN is expanding to ('__declspec(noreturn)').

If that is the case it's a generic bug because this patch set did not add the NORETURN macro.


On Tue, Dec 26, 2017 at 5:51 PM, Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>> wrote:
Michael:
  I suggest to add comments for each definition although there is redundant.

  Besides, after I apply these three patches, and build MdePkg.dsc with VS2015x86. It will report below error. Could you help look it?

        "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Vc\bin\cl.exe" /Foc:\r9tips\allpkg\edk2\Build\Mde\DEBUG_VS2015x86\IA32\MdePkg\Library\BaseCacheMaintenanceLib\BaseCacheMaintenanceLib\OUTPUT\.\X86Cache.obj /nologo /arch:IA32 /c /WX /GS- /W4 /Gs32768 /D UNICODE /O1b2 /GL /FIAutoGen.h /EHs-c- /GR- /GF /Gy /Zi /Gm /Gw -D DISABLE_NEW_DEPRECATED_INTERFACES /Ic:\r9tips\allpkg\edk2\MdePkg\Library\BaseCacheMaintenanceLib  /Ic:\r9tips\allpkg\edk2\Build\Mde\DEBUG_VS2015x86\IA32\MdePkg\Library\BaseCacheMaintenanceLib\BaseCacheMaintenanceLib\DEBUG  /Ic:\r9tips\allpkg\edk2\MdePkg  /Ic:\r9tips\allpkg\edk2\MdePkg\Include  /Ic:\r9tips\allpkg\edk2\MdePkg\Include\Ia32 c:\r9tips\allpkg\edk2\MdePkg\Library\BaseCacheMaintenanceLib\X86Cache.c
X86Cache.c
c:\r9tips\allpkg\edk2\MdePkg\Include\Library/BaseLib.h(4933): error C2059: syntax error: 'type'
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio 14.0\Vc\bin\cl.exe"' : return code '0x2'
Stop.

Thanks
Liming
From: Michael Zimmermann [mailto:sigmaepsilon92@gmail.com<mailto:sigmaepsilon92@gmail.com>]
Sent: Monday, December 25, 2017 11:50 PM
To: Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>

Subject: Re: [edk2] [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute

Liming:
The other macros have comments both before the compiler directives and before each define for each compiler.
To me it looks like these are slightly differently formulated only and kinda redundant too.
Is there a rule or do you have suggestions for writing comments for this kind of macro?

On Mon, Dec 25, 2017 at 4:11 AM, Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>> wrote:
Micha:
   Could you add comments for new macro RETURNS_TWICE like others, such as ANALYZER_NORETURN?

>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>]
>Sent: Friday, December 22, 2017 11:24 PM
>To: M1cha <sigmaepsilon92@gmail.com<mailto:sigmaepsilon92@gmail.com>>
>Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kinney, Michael D
><michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>
>Subject: Re: [edk2] [PATCH 1/3] MdePkg: add RETURNS_TWICE attribute
>
>On 22 December 2017 at 07:23, M1cha <sigmaepsilon92@gmail.com<mailto:sigmaepsilon92@gmail.com>> wrote:
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com<mailto:sigmaepsilon92@gmail.com>>
>> ---
>>  MdePkg/Include/Base.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
>> index 22ab5d3715fb..c863de407418 100644
>> --- a/MdePkg/Include/Base.h
>> +++ b/MdePkg/Include/Base.h
>> @@ -218,6 +218,16 @@ VERIFY_SIZE_OF (__VERIFY_UINT32_ENUM_SIZE,
>4);
>>    #endif
>>  #endif
>>
>> +#ifndef RETURNS_TWICE
>> +  #if defined (__GNUC__) || defined (__clang__)
>> +    #define RETURNS_TWICE  __attribute__((returns_twice))
>> +  #elif defined(_MSC_EXTENSIONS) && !defined(MDE_CPU_EBC)
>> +    #define RETURNS_TWICE
>> +  #else
>> +    #define RETURNS_TWICE
>
>What is the point of having two versions that are #defined to nothing?
>
>> +  #endif
>> +#endif
>> +
>>  //
>>  // For symbol name in assembly code, an extra "_" is sometimes necessary
>>  //
>> --
>> 2.15.1
>>

[PATCH 2/3] MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump

[M1cha]

When compiling with any ARM toolchain and Os, registers can get
trashed when returning for the second time from SetJump because GCC
only handles this correctly when using standard names like 'setjmp' or
'getcontext'. When different names are used you have to use the
attribute 'returns_twice' to tell gcc to be extra careful.

example:
extern int  FN_NAME(void*);

void jmp_buf_set(void *jmpb, void (*f)(void))
{
  if (!FN_NAME(jmpb))
    f();
}

this code produces this wrong code with Os:
00000000 <jmp_buf_set>:
   0: e92d4010 push {r4, lr}
   4: e1a04001 mov r4, r1
   8: ebfffffe bl 0 <nonstandard_setjmp>
   c: e3500000 cmp r0, #0
  10: 01a03004 moveq r3, r4
  14: 08bd4010 popeq {r4, lr}
  18: 012fff13 bxeq r3
  1c: e8bd4010 pop {r4, lr}
  20: e12fff1e bx lr

The generated code pushes backups of r4 and lr to the stack and then
saves all registers using nonstandard_setjmp.
Then it pops the stack and jumps to the function in r3 which is the
main problem because now the function can overwrite our register
backups on the stack.
When we return a second time from the call to nonstandard_setjmp, the
stack pointer has it's original(pushed) position and when the code
pops r4 and lr from the stack the values are not guaranteed to be the
same.

When using a standard name like setjmp or getcontext or adding
'__attribute__((returns_twice))' to nonstandard_setjmp's declaration
the code looks different:

00000000 <jmp_buf_set>:
   0: e92d4007 push {r0, r1, r2, lr}
   4: e58d1004 str r1, [sp, #4]
   8: ebfffffe bl 0 <setjmp>
   c: e3500000 cmp r0, #0
  10: 059d3004 ldreq r3, [sp, #4]
  14: 01a0e00f moveq lr, pc
  18: 012fff13 bxeq r3
  1c: e28dd00c add sp, sp, #12
  20: e49de004 pop {lr} ; (ldr lr, [sp], #4)
  24: e12fff1e bx lr

Here the problem is being solved by restoring r3 from the stack
without popping it.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
---
 MdePkg/Include/Library/BaseLib.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 2b98af4cd17e..10976032adaa 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -4905,6 +4905,7 @@ MemoryFence (
 **/
 UINTN
 EFIAPI
+RETURNS_TWICE
 SetJump (
   OUT     BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer
   );
-- 
2.15.1

Re: [PATCH 2/3] MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to SetJump

[Ard Biesheuvel]

On 22 December 2017 at 07:23, M1cha <sigmaepsilon92@gmail.com> wrote:
> When compiling with any ARM toolchain and Os, registers can get
> trashed when returning for the second time from SetJump because GCC
> only handles this correctly when using standard names like 'setjmp' or
> 'getcontext'. When different names are used you have to use the
> attribute 'returns_twice' to tell gcc to be extra careful.
>
> example:
> extern int  FN_NAME(void*);
>
> void jmp_buf_set(void *jmpb, void (*f)(void))
> {
>   if (!FN_NAME(jmpb))
>     f();
> }
>
> this code produces this wrong code with Os:
> 00000000 <jmp_buf_set>:
>    0: e92d4010 push {r4, lr}
>    4: e1a04001 mov r4, r1
>    8: ebfffffe bl 0 <nonstandard_setjmp>
>    c: e3500000 cmp r0, #0
>   10: 01a03004 moveq r3, r4
>   14: 08bd4010 popeq {r4, lr}
>   18: 012fff13 bxeq r3
>   1c: e8bd4010 pop {r4, lr}
>   20: e12fff1e bx lr
>
> The generated code pushes backups of r4 and lr to the stack and then
> saves all registers using nonstandard_setjmp.
> Then it pops the stack and jumps to the function in r3 which is the
> main problem because now the function can overwrite our register
> backups on the stack.
> When we return a second time from the call to nonstandard_setjmp, the
> stack pointer has it's original(pushed) position and when the code
> pops r4 and lr from the stack the values are not guaranteed to be the
> same.
>
> When using a standard name like setjmp or getcontext or adding
> '__attribute__((returns_twice))' to nonstandard_setjmp's declaration
> the code looks different:
>
> 00000000 <jmp_buf_set>:
>    0: e92d4007 push {r0, r1, r2, lr}
>    4: e58d1004 str r1, [sp, #4]
>    8: ebfffffe bl 0 <setjmp>
>    c: e3500000 cmp r0, #0
>   10: 059d3004 ldreq r3, [sp, #4]
>   14: 01a0e00f moveq lr, pc
>   18: 012fff13 bxeq r3
>   1c: e28dd00c add sp, sp, #12
>   20: e49de004 pop {lr} ; (ldr lr, [sp], #4)
>   24: e12fff1e bx lr
>
> Here the problem is being solved by restoring r3 from the stack
> without popping it.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>

Thanks for fixing this. Note that this affects all architectures
targeted by Clang/GCC, not just 32-bit ARM.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  MdePkg/Include/Library/BaseLib.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
> index 2b98af4cd17e..10976032adaa 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -4905,6 +4905,7 @@ MemoryFence (
>  **/
>  UINTN
>  EFIAPI
> +RETURNS_TWICE
>  SetJump (
>    OUT     BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer
>    );
> --
> 2.15.1
>

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

[M1cha]

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>
---
 MdePkg/Include/Library/BaseLib.h          | 1 +
 MdePkg/Library/BaseLib/BaseLibInternals.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 10976032adaa..e2eb46da4584 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -4929,6 +4929,7 @@ SetJump (
 **/
 VOID
 EFIAPI
+NORETURN
 LongJump (
   IN      BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer,
   IN      UINTN                     Value
diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h b/MdePkg/Library/BaseLib/BaseLibInternals.h
index 9dca97a0dcc9..3cd5fe34fc1b 100644
--- a/MdePkg/Library/BaseLib/BaseLibInternals.h
+++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
@@ -442,6 +442,7 @@ InternalAssertJumpBuffer (
 
 **/
 VOID
+NORETURN
 EFIAPI
 InternalLongJump (
   IN      BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer,
-- 
2.15.1

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

[Ard Biesheuvel]

On 22 December 2017 at 07:23, M1cha <sigmaepsilon92@gmail.com> wrote:
> 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>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  MdePkg/Include/Library/BaseLib.h          | 1 +
>  MdePkg/Library/BaseLib/BaseLibInternals.h | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
> index 10976032adaa..e2eb46da4584 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -4929,6 +4929,7 @@ SetJump (
>  **/
>  VOID
>  EFIAPI
> +NORETURN
>  LongJump (
>    IN      BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer,
>    IN      UINTN                     Value
> diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h b/MdePkg/Library/BaseLib/BaseLibInternals.h
> index 9dca97a0dcc9..3cd5fe34fc1b 100644
> --- a/MdePkg/Library/BaseLib/BaseLibInternals.h
> +++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
> @@ -442,6 +442,7 @@ InternalAssertJumpBuffer (
>
>  **/
>  VOID
> +NORETURN
>  EFIAPI
>  InternalLongJump (
>    IN      BASE_LIBRARY_JUMP_BUFFER  *JumpBuffer,
> --
> 2.15.1
>

Re: [PATCH 0/3] fix GCC optimizations and warnings for SetJump/LongJump

[Kinney, Michael D]

I see the .h file updates here.

I think the C files in MdePkg/Library/BaseLib also need
to be updated for this patch series to be complete.

Thanks,

Mike

> -----Original Message-----
> From: M1cha [mailto:sigmaepsilon92@gmail.com]
> Sent: Thursday, December 21, 2017 11:24 PM
> 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 0/3] fix GCC optimizations and
> warnings for SetJump/LongJump
> 
> I've already discussed this in past but never actually
> sent proper
> patches for some reason.
> 
> This patch series is about fixing problems with these
> functions when
> using GCC.
> 
> M1cha (3):
>   MdePkg: add RETURNS_TWICE attribute
>   MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to
> SetJump
>   MdePkg: add NORETURN attribute to LongJump and
> InternalLongJump
> 
>  MdePkg/Include/Base.h                     | 10
> ++++++++++
>  MdePkg/Include/Library/BaseLib.h          |  2 ++
>  MdePkg/Library/BaseLib/BaseLibInternals.h |  1 +
>  3 files changed, 13 insertions(+)
> 
> --
> 2.15.1

Re: [PATCH 0/3] fix GCC optimizations and warnings for SetJump/LongJump

[Michael Zimmermann]

Is adding attributes to both header and source files an UEFI coding
convention?
Because for the compiler it's only necessary to do that in the header files
afaik.

Thanks
Michael

On Fri, Dec 22, 2017 at 6:55 PM, Kinney, Michael D <
michael.d.kinney@intel.com> wrote:

> I see the .h file updates here.
>
> I think the C files in MdePkg/Library/BaseLib also need
> to be updated for this patch series to be complete.
>
> Thanks,
>
> Mike
>
> > -----Original Message-----
> > From: M1cha [mailto:sigmaepsilon92@gmail.com]
> > Sent: Thursday, December 21, 2017 11:24 PM
> > 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 0/3] fix GCC optimizations and
> > warnings for SetJump/LongJump
> >
> > I've already discussed this in past but never actually
> > sent proper
> > patches for some reason.
> >
> > This patch series is about fixing problems with these
> > functions when
> > using GCC.
> >
> > M1cha (3):
> >   MdePkg: add RETURNS_TWICE attribute
> >   MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to
> > SetJump
> >   MdePkg: add NORETURN attribute to LongJump and
> > InternalLongJump
> >
> >  MdePkg/Include/Base.h                     | 10
> > ++++++++++
> >  MdePkg/Include/Library/BaseLib.h          |  2 ++
> >  MdePkg/Library/BaseLib/BaseLibInternals.h |  1 +
> >  3 files changed, 13 insertions(+)
> >
> > --
> > 2.15.1
>
>

Re: [PATCH 0/3] fix GCC optimizations and warnings for SetJump/LongJump

[Kinney, Michael D]

Michael,

For many aspects of a function declaration and function implementation the C
compiler requires the headers to match.  I think it is good practice to have them match.

In general, when implementing a library instance, you can take a copy of the library
class .h file into a library instance .c file and fill in the body of the functions.  When
using this approach, the function headers are always identical.

Mike

From: Michael Zimmermann [mailto:sigmaepsilon92@gmail.com]
Sent: Friday, December 22, 2017 10:27 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: edk2-devel@lists.01.org; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>
Subject: Re: [edk2] [PATCH 0/3] fix GCC optimizations and warnings for SetJump/LongJump

Is adding attributes to both header and source files an UEFI coding convention?
Because for the compiler it's only necessary to do that in the header files afaik.

Thanks
Michael

On Fri, Dec 22, 2017 at 6:55 PM, Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:
I see the .h file updates here.

I think the C files in MdePkg/Library/BaseLib also need
to be updated for this patch series to be complete.

Thanks,

Mike

> -----Original Message-----
> From: M1cha [mailto:sigmaepsilon92@gmail.com<mailto:sigmaepsilon92@gmail.com>]
> Sent: Thursday, December 21, 2017 11:24 PM
> To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>; Kinney,
> Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming
> <liming.gao@intel.com<mailto:liming.gao@intel.com>>
> Subject: [edk2] [PATCH 0/3] fix GCC optimizations and
> warnings for SetJump/LongJump
>
> I've already discussed this in past but never actually
> sent proper
> patches for some reason.
>
> This patch series is about fixing problems with these
> functions when
> using GCC.
>
> M1cha (3):
>   MdePkg: add RETURNS_TWICE attribute
>   MdePkg/BaseLib: add attribute 'RETURNS_TWICE' to
> SetJump
>   MdePkg: add NORETURN attribute to LongJump and
> InternalLongJump
>
>  MdePkg/Include/Base.h                     | 10
> ++++++++++
>  MdePkg/Include/Library/BaseLib.h          |  2 ++
>  MdePkg/Library/BaseLib/BaseLibInternals.h |  1 +
>  3 files changed, 13 insertions(+)
>
> --
> 2.15.1