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 > > > 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
