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 > > 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 <marvin.haeu...@outlook.com> > > --- > > 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 @@ > > **/ > > VOID > > EFIAPI > > +ANALYZER_NORETURN > > CpuBreakpoint ( > > VOID > > ) > > { > > __break (0); > > + > > + ANALYZER_UNREACHABLE (); > > } > > > > /** > > diff --git a/MdePkg/Library/BaseLib/Ipf/CpuBreakpointMsc.c > > b/MdePkg/Library/BaseLib/Ipf/CpuBreakpointMsc.c > > index 89b0acfd801b..2b098d462bee 100644 > > --- a/MdePkg/Library/BaseLib/Ipf/CpuBreakpointMsc.c > > +++ b/MdePkg/Library/BaseLib/Ipf/CpuBreakpointMsc.c > > @@ -29,11 +29,14 @@ > > **/ > > VOID > > EFIAPI > > +ANALYZER_NORETURN > > CpuBreakpoint ( > > VOID > > ) > > { > > __break (0); > > + > > + ANALYZER_UNREACHABLE (); > > } > > > > /** > > diff --git a/MdePkg/Library/BaseLib/X64/CpuBreakpoint.c > > b/MdePkg/Library/BaseLib/X64/CpuBreakpoint.c > > index d654f845716f..72055c2acfaa 100644 > > --- a/MdePkg/Library/BaseLib/X64/CpuBreakpoint.c > > +++ b/MdePkg/Library/BaseLib/X64/CpuBreakpoint.c > > @@ -30,10 +30,13 @@ void __debugbreak (); **/ VOID EFIAPI > > +ANALYZER_NORETURN > > CpuBreakpoint ( > > VOID > > ) > > { > > __debugbreak (); > > + > > + ANALYZER_UNREACHABLE (); > > } > > > > diff --git a/MdePkg/Library/BaseLib/X64/GccInline.c > > b/MdePkg/Library/BaseLib/X64/GccInline.c > > index 3d175ee9314e..b3443c55cb27 100644 > > --- a/MdePkg/Library/BaseLib/X64/GccInline.c > > +++ b/MdePkg/Library/BaseLib/X64/GccInline.c > > @@ -99,11 +99,14 @@ CpuPause ( > > **/ > > VOID > > EFIAPI > > +ANALYZER_NORETURN > > CpuBreakpoint ( > > VOID > > ) > > { > > __asm__ __volatile__ ("int $3"); > > + > > + ANALYZER_UNREACHABLE (); > > } > > > > > > diff --git a/MdePkg/Include/Library/BaseLib.h > > b/MdePkg/Include/Library/BaseLib.h > > index 79f421a97111..87f1e39d61db 100644 > > --- a/MdePkg/Include/Library/BaseLib.h > > +++ b/MdePkg/Include/Library/BaseLib.h > > @@ -3960,6 +3960,7 @@ SwitchStack ( > > **/ > > VOID > > EFIAPI > > +ANALYZER_NORETURN > > CpuBreakpoint ( > > VOID > > ); > > @@ -3976,6 +3977,7 @@ CpuBreakpoint ( > > **/ > > VOID > > EFIAPI > > +ANALYZER_NORETURN > > CpuDeadLoop ( > > VOID > > ); > > diff --git a/MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S > > b/MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S > > index 6323cffaa981..e4008ec1d3fd 100644 > > --- a/MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S > > +++ b/MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S > > @@ -28,6 +28,7 @@ GCC_ASM_EXPORT(CpuBreakpoint) #**/ #VOID > > #EFIAPI > > +#ANALYZER_NORETURN > > #CpuBreakpoint ( > > # VOID > > # ); > > diff --git a/MdePkg/Library/BaseLib/Arm/CpuBreakpoint.S > > b/MdePkg/Library/BaseLib/Arm/CpuBreakpoint.S > > index b6b80a1326d0..accb6f1b3287 100644 > > --- a/MdePkg/Library/BaseLib/Arm/CpuBreakpoint.S > > +++ b/MdePkg/Library/BaseLib/Arm/CpuBreakpoint.S > > @@ -27,6 +27,7 @@ GCC_ASM_EXPORT(CpuBreakpoint) #**/ #VOID > > #EFIAPI > > +#ANALYZER_NORETURN > > #CpuBreakpoint ( > > # VOID > > # ); > > diff --git a/MdePkg/Library/BaseLib/Arm/CpuBreakpoint.asm > > b/MdePkg/Library/BaseLib/Arm/CpuBreakpoint.asm > > index 8a8065159bf2..5d51d13930f3 100644 > > --- a/MdePkg/Library/BaseLib/Arm/CpuBreakpoint.asm > > +++ b/MdePkg/Library/BaseLib/Arm/CpuBreakpoint.asm > > @@ -27,6 +27,7 @@ > > ;**/ > > ;VOID > > ;EFIAPI > > +;ANALYZER_NORETURN > > ;CpuBreakpoint ( > > ; VOID > > ; ); > > diff --git a/MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.asm > > b/MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.asm > > index e4364055e8ee..e48a0fa92517 100644 > > --- a/MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.asm > > +++ b/MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.asm > > @@ -28,6 +28,7 @@ > > > > ;------------------------------------------------------------------------------ > > ; VOID > > ; EFIAPI > > +; ANALYZER_NORETURN > > ; CpuBreakpoint ( > > ; VOID > > ; ); > > diff --git a/MdePkg/Library/BaseLib/X64/CpuBreakpoint.asm > > b/MdePkg/Library/BaseLib/X64/CpuBreakpoint.asm > > index 25dd9b48e808..f0e4e93714fb 100644 > > --- a/MdePkg/Library/BaseLib/X64/CpuBreakpoint.asm > > +++ b/MdePkg/Library/BaseLib/X64/CpuBreakpoint.asm > > @@ -25,6 +25,7 @@ > > > > ;------------------------------------------------------------------------------ > > ; VOID > > ; EFIAPI > > +; ANALYZER_NORETURN > > ; CpuBreakpoint ( > > ; VOID > > ; ); > > -- > > 2.7.4.windows.1 > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
