Re: [edk2] [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and CpuBreakpoint() as ANALYZER_NORETURN.

[Gao, Liming]

Marvin:
  This is my mistake. It should be Base.h. I agree your proposal to create 
three patches.

Thanks
Liming
From: Marvin Häuser [mailto:marvin.haeu...@outlook.com]
Sent: Tuesday, June 14, 2016 2:05 AM
To: edk2-devel@lists.01.org
Cc: Gao, Liming 
Subject: RE: [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and 
CpuBreakpoint() as ANALYZER_NORETURN.

Liming,

UNREACHABLE() in fact may have an impact as it is a signal that the code flow 
cannot reach that position. Thus a compiler or analyzer may classify everything 
following as unreachable and issue warnings about it or optimize it away. For 
stack cleanups, as Andrew suggested, this is a good thing, not-so for code 
following that instruction though - hence this patch was a bad idea.

I believe the defines are better off in Base.h, as other compiler-specific 
things (UEFI type defines, GLOBAL_REMOVE_IF_UNREFERENCED, ...) are there as 
well in contrast to BaseLib.h, which is basically an unrelated library. These 
macros may not only be used for ASSERT() but, as Andrew suggested, also in the 
handoff functions of all phase modules and these would all need to include 
BaseLib.h to get the necessary defines according to your suggestion.

In the end, it is up to you whether the defines will be in Base.h or BaseLib.h 
- or if regular and ANALYZER_ macros are one or separate patches -, but for 
both cases I do not understand the intention and consider one patch adding the 
defines to Base.h (as already submitted and still backed by myself), another 
patch adding the ANALYZER_UNREACHABLE() decoration to the end of the ASSERT 
if-branch (which will arrive once my concerns regarding the first patch have 
been addressed) and finally a third patch to add the decorations to the phase 
transition functions as pointed out by Andrew as the best solution.

Thanks for your time,
Marvin.

> -Original Message-
> From: Gao, Liming [mailto:liming@intel.com]
> Sent: Monday, June 13, 2016 4:33 AM
> To: Marvin Häuser ; edk2-
> de...@lists.01.org
> Cc: Kinney, Michael D
> Subject: RE: [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and
> CpuBreakpoint() as ANALYZER_NORETURN.
>
> Marvin:
> This is a better. I suggest to create two patch sets. I think you focuses on 
> the
> first. For the second, UNREACHABLE is a decorator, and has no real impact.
> 1. For static code analyzer, add ANALYZER_UNREACHABLE macro in
> BaseLib.h, and use it in _ASSERT() macro in DebugLib.h 2. For un reachable
> case, add UNREACHABLE macro in BaseLib.h, and apply it in code pointed by
> Andrew.
>
> Thanks
> Liming
> > -Original Message-
> > From: Marvin Häuser [mailto:marvin.haeu...@outlook.com]
> > Sent: Monday, June 13, 2016 2:46 AM
> > To: edk2-devel@lists.01.org
> > Cc: Kinney, Michael D ; Gao, Liming
> >
> > Subject: RE: [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and
> > CpuBreakpoint() as ANALYZER_NORETURN.
> >
> > Dear package maintainers and reviewers,
> >
> > Please do not approve this patch yet. I have thought further about the
> > consequences of decorating CpuDeadLoop() and CpuBreakpoint() with the
> > ANALYZER_NORETURN attribute and fear that the Analyzer, against the
> > original intention, could issue incorrect warnings when they are used
> > in the middle of code. All code following could probably be classified
> > as unreachable (as the Analyzer will rightfully assume the Cpu* functions
> won't return).
> >
> > I do not know if there is a way to limit this behavior to ASSERT.
> > Maybe this patch should be gotten rid of and UNREACHABLE() should be
> > added within the ASSERT() macro itself? This would make more sense
> > than what I have done in this patch, looking back.
> >
> > If you have thoughts, please be kind enough to share them.
> >
> > Regards,
> > Marvin.
> >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> > > Of Marvin Häuser
> > > Sent: Friday, June 10, 2016 11:02 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: michael.d.kin...@intel.com; 
> > > liming@intel.com
> > > Subject: [edk2] [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop()
> > > and
> > > CpuBreakpoint() as ANALYZER_NORETURN.
> > >
> > > Add the ANALYZER_NORETURN attribute to CpuDeadLoop() and
> > > CpuBreakpoint() to avoid false 'possible NULL-dereference' warnings
> > > when dereferencing pointers after having validated them with
> > > ASSERT(). As the ANALYZER-prefixed versions are being used, the code
> > > following the calls
> > will
> > > not be optimized away.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Marvin Haeuser
> > > ---
> > > MdePkg/Library/BaseLib/CpuDeadLoop.c | 3 +++
> > > MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c | 3 +++
> > > MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c | 3 +++
> 

Re: [edk2] [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and CpuBreakpoint() as ANALYZER_NORETURN.

[Kinney, Michael D]

Marvin,

I agree that Base.h is the right place for these new macros.

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Marvin 
> Häuser
> Sent: Monday, June 13, 2016 11:05 AM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming@intel.com>
> Subject: Re: [edk2] [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and
> CpuBreakpoint() as ANALYZER_NORETURN.
> 
> Liming,
> 
> UNREACHABLE() in fact may have an impact as it is a signal that the code flow 
> cannot
> reach that position. Thus a compiler or analyzer may classify everything 
> following as
> unreachable and issue warnings about it or optimize it away. For stack 
> cleanups, as
> Andrew suggested, this is a good thing, not-so for code following that 
> instruction
> though - hence this patch was a bad idea.
> 
> I believe the defines are better off in Base.h, as other compiler-specific 
> things (UEFI
> type defines, GLOBAL_REMOVE_IF_UNREFERENCED, ...) are there as well in 
> contrast to
> BaseLib.h, which is basically an unrelated library. These macros may not only 
> be used
> for ASSERT() but, as Andrew suggested, also in the handoff functions of all 
> phase
> modules and these would all need to include BaseLib.h to get the necessary 
> defines
> according to your suggestion.
> 
> In the end, it is up to you whether the defines will be in Base.h or 
> BaseLib.h - or if
> regular and ANALYZER_ macros are one or separate patches -, but for both 
> cases I do not
> understand the intention and consider one patch adding the defines to Base.h 
> (as
> already submitted and still backed by myself), another patch adding the
> ANALYZER_UNREACHABLE() decoration to the end of the ASSERT if-branch (which 
> will arrive
> once my concerns regarding the first patch have been addressed) and finally a 
> third
> patch to add the decorations to the phase transition functions as pointed out 
> by Andrew
> as the best solution.
> 
> Thanks for your time,
> Marvin.
> 
> > -Original Message-
> > From: Gao, Liming [mailto:liming@intel.com]
> > Sent: Monday, June 13, 2016 4:33 AM
> > To: Marvin Häuser <marvin.haeu...@outlook.com>; edk2-
> > de...@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>
> > Subject: RE: [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and
> > CpuBreakpoint() as ANALYZER_NORETURN.
> >
> > Marvin:
> >   This is a better. I suggest to create two patch sets. I think you focuses 
> > on the
> > first. For the second, UNREACHABLE is a decorator, and has no real impact.
> > 1. For static code analyzer, add ANALYZER_UNREACHABLE macro in
> > BaseLib.h, and use it in _ASSERT() macro in DebugLib.h 2. For un reachable
> > case, add UNREACHABLE macro in BaseLib.h, and apply it in code pointed by
> > Andrew.
> >
> > Thanks
> > Liming
> > > -Original Message-
> > > From: Marvin Häuser [mailto:marvin.haeu...@outlook.com]
> > > Sent: Monday, June 13, 2016 2:46 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming
> > > <liming@intel.com>
> > > Subject: RE: [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and
> > > CpuBreakpoint() as ANALYZER_NORETURN.
> > >
> > > Dear package maintainers and reviewers,
> > >
> > > Please do not approve this patch yet. I have thought further about the
> > > consequences of decorating CpuDeadLoop() and CpuBreakpoint() with the
> > > ANALYZER_NORETURN attribute and fear that the Analyzer, against the
> > > original intention, could issue incorrect warnings when they are used
> > > in the middle of code. All code following could probably be classified
> > > as unreachable (as the Analyzer will rightfully assume the Cpu* functions
> > won't return).
> > >
> > > I do not know if there is a way to limit this behavior to ASSERT.
> > > Maybe this patch should be gotten rid of and UNREACHABLE() should be
> > > added within the ASSERT() macro itself? This would make more sense
> > > than what I have done in this patch, looking back.
> > >
> > > If you have thoughts, please be kind enough to share them.
> > >
> > > Regards,
> > > Marvin.
> > >
> > > > -Original Message-
> > > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> > > > Of Marvin Häuser
> > > > Sent: Friday, June 10, 2016 11:02 PM
> > > > To: edk2-devel@lists.01.org
> >

Re: [edk2] [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and CpuBreakpoint() as ANALYZER_NORETURN.

[Marvin Häuser]

Liming,

UNREACHABLE() in fact may have an impact as it is a signal that the code flow 
cannot reach that position. Thus a compiler or analyzer may classify everything 
following as unreachable and issue warnings about it or optimize it away. For 
stack cleanups, as Andrew suggested, this is a good thing, not-so for code 
following that instruction though - hence this patch was a bad idea.

I believe the defines are better off in Base.h, as other compiler-specific 
things (UEFI type defines, GLOBAL_REMOVE_IF_UNREFERENCED, ...) are there as 
well in contrast to BaseLib.h, which is basically an unrelated library. These 
macros may not only be used for ASSERT() but, as Andrew suggested, also in the 
handoff functions of all phase modules and these would all need to include 
BaseLib.h to get the necessary defines according to your suggestion.

In the end, it is up to you whether the defines will be in Base.h or BaseLib.h 
- or if regular and ANALYZER_ macros are one or separate patches -, but for 
both cases I do not understand the intention and consider one patch adding the 
defines to Base.h (as already submitted and still backed by myself), another 
patch adding the ANALYZER_UNREACHABLE() decoration to the end of the ASSERT 
if-branch (which will arrive once my concerns regarding the first patch have 
been addressed) and finally a third patch to add the decorations to the phase 
transition functions as pointed out by Andrew as the best solution.

Thanks for your time,
Marvin.

> -Original Message-
> From: Gao, Liming [mailto:liming@intel.com]
> Sent: Monday, June 13, 2016 4:33 AM
> To: Marvin Häuser ; edk2-
> de...@lists.01.org
> Cc: Kinney, Michael D 
> Subject: RE: [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and
> CpuBreakpoint() as ANALYZER_NORETURN.
> 
> Marvin:
>   This is a better. I suggest to create two patch sets. I think you focuses 
> on the
> first. For the second, UNREACHABLE is a decorator, and has no real impact.
> 1. For static code analyzer, add ANALYZER_UNREACHABLE macro in
> BaseLib.h, and use it in _ASSERT() macro in DebugLib.h 2. For un reachable
> case, add UNREACHABLE macro in BaseLib.h, and apply it in code pointed by
> Andrew.
> 
> Thanks
> Liming
> > -Original Message-
> > From: Marvin Häuser [mailto:marvin.haeu...@outlook.com]
> > Sent: Monday, June 13, 2016 2:46 AM
> > To: edk2-devel@lists.01.org
> > Cc: Kinney, Michael D ; Gao, Liming
> > 
> > Subject: RE: [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and
> > CpuBreakpoint() as ANALYZER_NORETURN.
> >
> > Dear package maintainers and reviewers,
> >
> > Please do not approve this patch yet. I have thought further about the
> > consequences of decorating CpuDeadLoop() and CpuBreakpoint() with the
> > ANALYZER_NORETURN attribute and fear that the Analyzer, against the
> > original intention, could issue incorrect warnings when they are used
> > in the middle of code. All code following could probably be classified
> > as unreachable (as the Analyzer will rightfully assume the Cpu* functions
> won't return).
> >
> > I do not know if there is a way to limit this behavior to ASSERT.
> > Maybe this patch should be gotten rid of and UNREACHABLE() should be
> > added within the ASSERT() macro itself? This would make more sense
> > than what I have done in this patch, looking back.
> >
> > If you have thoughts, please be kind enough to share them.
> >
> > Regards,
> > Marvin.
> >
> > > -Original Message-
> > > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf
> > > Of Marvin Häuser
> > > Sent: Friday, June 10, 2016 11:02 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: michael.d.kin...@intel.com; liming@intel.com
> > > Subject: [edk2] [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop()
> > > and
> > > CpuBreakpoint() as ANALYZER_NORETURN.
> > >
> > > Add the ANALYZER_NORETURN attribute to CpuDeadLoop() and
> > > CpuBreakpoint() to avoid false 'possible NULL-dereference' warnings
> > > when dereferencing pointers after having validated them with
> > > ASSERT(). As the ANALYZER-prefixed versions are being used, the code
> > > following the calls
> > will
> > > not be optimized away.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Marvin Haeuser 
> > > ---
> > >  MdePkg/Library/BaseLib/CpuDeadLoop.c   | 3 +++
> > >  MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c | 3 +++
> > >  MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c| 3 +++
> > >  MdePkg/Library/BaseLib/Ipf/CpuBreakpoint.c | 3 +++
> > >  MdePkg/Library/BaseLib/Ipf/CpuBreakpointMsc.c  | 3 +++
> > >  MdePkg/Library/BaseLib/X64/CpuBreakpoint.c | 3 +++
> > >  MdePkg/Library/BaseLib/X64/GccInline.c | 3 +++
> > >  MdePkg/Include/Library/BaseLib.h   | 2 ++
> > >  MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S | 1 +
> > >  

Re: [edk2] [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and CpuBreakpoint() as ANALYZER_NORETURN.

[Gao, Liming]

Marvin:
  This is a better. I suggest to create two patch sets. I think you focuses on 
the first. For the second, UNREACHABLE is a decorator, and has no real impact. 
1. For static code analyzer, add ANALYZER_UNREACHABLE macro in BaseLib.h, and 
use it in _ASSERT() macro in DebugLib.h
2. For un reachable case, add UNREACHABLE macro in BaseLib.h, and apply it in 
code pointed by Andrew. 

Thanks
Liming
> -Original Message-
> From: Marvin Häuser [mailto:marvin.haeu...@outlook.com]
> Sent: Monday, June 13, 2016 2:46 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D ; Gao, Liming
> 
> Subject: RE: [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and
> CpuBreakpoint() as ANALYZER_NORETURN.
> 
> Dear package maintainers and reviewers,
> 
> Please do not approve this patch yet. I have thought further about the
> consequences of decorating CpuDeadLoop() and CpuBreakpoint() with the
> ANALYZER_NORETURN attribute and fear that the Analyzer, against the
> original intention, could issue incorrect warnings when they are used in the
> middle of code. All code following could probably be classified as unreachable
> (as the Analyzer will rightfully assume the Cpu* functions won't return).
> 
> I do not know if there is a way to limit this behavior to ASSERT. Maybe this
> patch should be gotten rid of and UNREACHABLE() should be added within
> the ASSERT() macro itself? This would make more sense than what I have
> done in this patch, looking back.
> 
> If you have thoughts, please be kind enough to share them.
> 
> Regards,
> Marvin.
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Marvin Häuser
> > Sent: Friday, June 10, 2016 11:02 PM
> > To: edk2-devel@lists.01.org
> > Cc: michael.d.kin...@intel.com; liming@intel.com
> > Subject: [edk2] [PATCH v1 2/2] MdePkg/BaseLib: Flag CpuDeadLoop() and
> > CpuBreakpoint() as ANALYZER_NORETURN.
> >
> > Add the ANALYZER_NORETURN attribute to CpuDeadLoop() and
> > CpuBreakpoint() to avoid false 'possible NULL-dereference' warnings when
> > dereferencing pointers after having validated them with ASSERT(). As the
> > ANALYZER-prefixed versions are being used, the code following the calls
> will
> > not be optimized away.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Marvin Haeuser 
> > ---
> >  MdePkg/Library/BaseLib/CpuDeadLoop.c   | 3 +++
> >  MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c | 3 +++
> >  MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c| 3 +++
> >  MdePkg/Library/BaseLib/Ipf/CpuBreakpoint.c | 3 +++
> >  MdePkg/Library/BaseLib/Ipf/CpuBreakpointMsc.c  | 3 +++
> >  MdePkg/Library/BaseLib/X64/CpuBreakpoint.c | 3 +++
> >  MdePkg/Library/BaseLib/X64/GccInline.c | 3 +++
> >  MdePkg/Include/Library/BaseLib.h   | 2 ++
> >  MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S | 1 +
> >  MdePkg/Library/BaseLib/Arm/CpuBreakpoint.S | 1 +
> >  MdePkg/Library/BaseLib/Arm/CpuBreakpoint.asm   | 1 +
> >  MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.asm  | 1 +
> >  MdePkg/Library/BaseLib/X64/CpuBreakpoint.asm   | 1 +
> >  13 files changed, 28 insertions(+)
> >
> > diff --git a/MdePkg/Library/BaseLib/CpuDeadLoop.c
> > b/MdePkg/Library/BaseLib/CpuDeadLoop.c
> > index 3f9440547d4d..7e386eab61a3 100644
> > --- a/MdePkg/Library/BaseLib/CpuDeadLoop.c
> > +++ b/MdePkg/Library/BaseLib/CpuDeadLoop.c
> > @@ -28,6 +28,7 @@
> >  **/
> >  VOID
> >  EFIAPI
> > +ANALYZER_NORETURN
> >  CpuDeadLoop (
> >VOID
> >)
> > @@ -35,4 +36,6 @@ CpuDeadLoop (
> >volatile UINTN  Index;
> >
> >for (Index = 0; Index == 0;);
> > +
> > +  ANALYZER_UNREACHABLE ();
> >  }
> > diff --git a/MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c
> > b/MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c
> > index 9b7d875664bd..bd8da8a671d1 100644
> > --- a/MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c
> > +++ b/MdePkg/Library/BaseLib/Ebc/CpuBreakpoint.c
> > @@ -29,11 +29,14 @@ _break (
> >  **/
> >  VOID
> >  EFIAPI
> > +ANALYZER_NORETURN
> >  CpuBreakpoint (
> >VOID
> >)
> >  {
> >_break (3);
> > +
> > +  ANALYZER_UNREACHABLE ();
> >  }
> >
> >  /**
> > diff --git a/MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c
> > b/MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c
> > index f5df7f2883c6..d68fd770ce3d 100644
> > --- a/MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c
> > +++ b/MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.c
> > @@ -32,10 +32,13 @@ void __debugbreak ();  **/  VOID  EFIAPI
> > +ANALYZER_NORETURN
> >  CpuBreakpoint (
> >VOID
> >)
> >  {
> >__debugbreak ();
> > +
> > +  ANALYZER_UNREACHABLE ();
> >  }
> >
> > diff --git a/MdePkg/Library/BaseLib/Ipf/CpuBreakpoint.c
> > b/MdePkg/Library/BaseLib/Ipf/CpuBreakpoint.c
> > index 302974bd5c98..29456b5f867d 100644
> > --- a/MdePkg/Library/BaseLib/Ipf/CpuBreakpoint.c
> > +++ b/MdePkg/Library/BaseLib/Ipf/CpuBreakpoint.c
> > @@ -24,11 +24,14 @@
> >