> On Jun 12, 2016, at 6:40 AM, Marvin Häuser <[email protected]> wrote: > > Hey Andrew, hey Liming, > > First, the more important patch is probably the Patch 2/2, which flags > CpuBreakpoint() and CpuDeadLoop() as ANALYZER_NORETURN to pretend to the > analyzer the functions can't exit for the sake of surpressing warnings. > I personally don't understand why the patch should be separated, as the two > pairs of macro and attribute have the very same purpose, just that one pair > is restricted to Analyzers. Also, the patch is quite small already in its > current state. > > Though I just thought about Patch 2/2 and it could actually be that someone > uses CpuBreakpoint() and CpuDeadLoop() outside of the ASSERT() macro and, if > it is not the last function call in a function, the Static Analyzer may issue > a warning about unreachable code. I have not thought about this before, to be > honest, and I am not certain on how to prevent this from happening. I would > value any kind of input from anyone! >
Marvin, I'm not sure what the right answer is but it feels like decorating the locations that do not return with something like UNREACHABLE() makes the most sense. https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/SecCore/SecMain.c#L253 https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c#L469 https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c#L432 https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c#L525 I don't think we have done it in the past as all these no return points are calls from one statically linked module to another statically linked module, so all the compiler knows is a C function is being called. So module A is calling module B. Thus A has the call from A to B, and B has the function but never calls it. Thanks, Andrew Fish > Thanks, > Marvin. > >> -----Original Message----- >> From: Gao, Liming [mailto:[email protected]] >> Sent: Sunday, June 12, 2016 5:22 AM >> To: [email protected] >> Cc: Marvin Häuser <[email protected]>; edk2- >> [email protected]; Kinney, Michael D <[email protected]> >> Subject: RE: [edk2] [PATCH v1 1/2] MdePkg: Add NORETURN attribute and >> UNREACHABLE() macro. >> >> Andrew: >> Thanks for your explanation. So, they are the different topics. I suggest to >> separate this patch. >> 1. NORETURN attribute and UNREACHABLE() macro will be used for the really >> NORETURN case, like phase change. >> 2. ANALYZER_UNREACHABLE and ANALYZER_NORETURN are for static code >> analyzer, like clang compiler analyzer. >> >> Thanks >> Liming >>> -----Original Message----- >>> From: [email protected] [mailto:[email protected]] >>> Sent: Sunday, June 12, 2016 11:11 AM >>> To: Gao, Liming <[email protected]> >>> Cc: Marvin Häuser <[email protected]>; edk2- >>> [email protected]; Kinney, Michael D <[email protected]> >>> Subject: Re: [edk2] [PATCH v1 1/2] MdePkg: Add NORETURN attribute and >>> UNREACHABLE() macro. >>> >>> >>>> On Jun 11, 2016, at 7:58 PM, Gao, Liming <[email protected]> wrote: >>>> >>>> Hi, >>>> I don't see the usage on NORETURN attribute and UNREACHABLE() >> macro. >>> Why we need to add them also? >>>> >>> >>> When you factor in debugging I think the only thing we have that are >>> really NORETURN would be phase changes. SEC calling PEI Core. DXE IPL >>> PEIM loading DXE Core, DXE Core entry point. I debug across and >>> ASSERT(), CpuBreakpoint(), or CpuDeadloop() all the time so turn that >>> compiler feature on would break stuff. For the mode transitions we >>> could save a few unreachable stack cleanup instructions? Thus I guess >>> things like the PEI Core entry point function are kind of NORETURN, >>> and I guess the UNREACHABLE would be like __builtin_unreachable() and >>> end up after the call to the PEI Core entry point. Given things like >>> the PEI Core entry point are defined in the PI spec, it would probably be >> better to define UNREACHABLE(). >>> >>> But I'm still not sure what problem we are trying to solve. >>> >>> Thanks, >>> >>> Andrew Fish >>> >>>> Thanks >>>> Liming >>>>> -----Original Message----- >>>>> From: Marvin Häuser [mailto:[email protected]] >>>>> Sent: Saturday, June 11, 2016 5:02 AM >>>>> To: [email protected] >>>>> Cc: Gao, Liming <[email protected]>; Kinney, Michael D >>>>> <[email protected]> >>>>> Subject: [PATCH v1 1/2] MdePkg: Add NORETURN attribute and >>>>> UNREACHABLE() macro. >>>>> >>>>> The NORETURN attribute informs compilers and analyzers that the >>>>> flagged function cannot return. This may improve the quality of the >> optimizations. >>>>> >>>>> The UNREACHABLE() macro informs compilers and analyzers that its >>> position >>>>> cannot be reached, for example eliminating implicit returns. >>>>> It is recommended to be used together with the NORETURN attribute >>>>> to prevent warnings regarding the function flagged as 'noreturn' >>>>> returning. >>>>> >>>>> The ANALYZER-prefixed versions have the same effects, but exclude >>>>> compilers. >>>>> They may be used to surpress warnings of static analyzers, such as >>> possible >>>>> dereferencing of a NULL pointer when dereferencing it after having >>> checked >>>>> it >>>>> via ASSERT(). >>>>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>> Signed-off-by: Marvin Haeuser <[email protected]> >>>>> --- >>>>> MdePkg/Include/Base.h | 103 ++++++++++++++++++++ >>>>> 1 file changed, 103 insertions(+) >>>>> >>>>> diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h index >>>>> 89b2aed07229..9dfafb282177 100644 >>>>> --- a/MdePkg/Include/Base.h >>>>> +++ b/MdePkg/Include/Base.h >>>>> @@ -85,6 +85,109 @@ VERIFY_SIZE_OF (CHAR16, 2); #endif >>>>> >>>>> // >>>>> +// Should be used in combination with NORETURN to avoid 'noreturn' >>>>> returns >>>>> +// warnings. >>>>> +// >>>>> +#ifndef UNREACHABLE >>>>> + #ifdef __GNUC__ >>>>> + /// >>>>> + /// Signal compilers and analyzers that this call is not reachable. >>>>> It is >>>>> + /// up to the compiler to remove any code past that point. >>>>> + /// >>>>> + #define UNREACHABLE() __builtin_unreachable() >>>>> + #elif defined (__has_feature) >>>>> + #if __has_builtin (__builtin_unreachable) >>>>> + /// >>>>> + /// Signal compilers and analyzers that this call is not >>>>> reachable. It is >>>>> + /// up to the compiler to remove any code past that point. >>>>> + /// >>>>> + #define UNREACHABLE() __builtin_unreachable() >>>>> + #endif >>>>> + #endif >>>>> + >>>>> + #ifndef UNREACHABLE >>>>> + /// >>>>> + /// Signal compilers and analyzers that this call is not reachable. >>>>> It is >>>>> + /// up to the compiler to remove any code past that point. >>>>> + /// >>>>> + #define UNREACHABLE() >>>>> + #endif >>>>> +#endif >>>>> + >>>>> +// >>>>> +// Signaling compilers and analyzers that a certain function >>>>> +cannot return >>>>> may >>>>> +// remove all following code and thus lead to better optimization >>>>> +and >>> less >>>>> +// false positives. >>>>> +// >>>>> +#ifndef NORETURN >>>>> + #ifdef __GNUC__ >>>>> + /// >>>>> + /// Signal compilers and analyzers that the function cannot return. >>>>> + /// It is up to the compiler to remove any code past a call to >>>>> functions >>>>> + /// flagged with this attribute. >>>>> + /// >>>>> + #define NORETURN __attribute__ ((noreturn)) >>>>> + #else >>>>> + /// >>>>> + /// Signal compilers and analyzers that the function cannot return. >>>>> + /// It is up to the compiler to remove any code past a call to >>>>> functions >>>>> + /// flagged with this attribute. >>>>> + /// >>>>> + #define NORETURN >>>>> + #endif >>>>> +#endif >>>>> + >>>>> +// >>>>> +// Should be used in combination with ANALYZER_NORETURN to avoid >>>>> 'noreturn' >>>>> +// returns warnings. >>>>> +// >>>>> +#ifndef ANALYZER_UNREACHABLE >>>>> + #ifdef __clang_analyzer__ >>>>> + #if __has_builtin (__builtin_unreachable) >>>>> + /// >>>>> + /// Signal the analyzer that this call is not reachable. >>>>> + /// This excludes compilers. >>>>> + /// >>>>> + #define ANALYZER_UNREACHABLE() __builtin_unreachable() >>>>> + #endif >>>>> + #endif >>>>> + >>>>> + #ifndef ANALYZER_UNREACHABLE >>>>> + /// >>>>> + /// Signal the analyzer that this call is not reachable. >>>>> + /// This excludes compilers. >>>>> + /// >>>>> + #define ANALYZER_UNREACHABLE() >>>>> + #endif >>>>> +#endif >>>>> + >>>>> +// >>>>> +// Static Analyzers may issue errors about potential >>>>> +NULL-dereferences >>>>> when >>>>> +// dereferencing a pointer, that has been checked before, outside >>>>> +of a // NULL-check. This may lead to false positives, such as >>>>> +when using >>> ASSERT() >>>>> +// for verification. >>>>> +// >>>>> +#ifndef ANALYZER_NORETURN >>>>> + #ifdef __has_feature >>>>> + #if __has_feature (attribute_analyzer_noreturn) >>>>> + /// >>>>> + /// Signal analyzers that the function cannot return. >>>>> + /// This excludes compilers. >>>>> + /// >>>>> + #define ANALYZER_NORETURN __attribute__ >> ((analyzer_noreturn)) >>>>> + #endif >>>>> + #endif >>>>> + >>>>> + #ifndef ANALYZER_NORETURN >>>>> + /// >>>>> + /// Signal the analyzer that the function cannot return. >>>>> + /// This excludes compilers. >>>>> + /// >>>>> + #define ANALYZER_NORETURN >>>>> + #endif >>>>> +#endif >>>>> + >>>>> +// >>>>> // For symbol name in assembly code, an extra "_" is sometimes >>> necessary >>>>> // >>>>> >>>>> -- >>>>> 2.7.4.windows.1 >>>> >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> [email protected] >>>> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
