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 > > > > 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
