Hello Andrew, Yes, I agree with you, these should have the attribute/macro call added in a separate patch once this one has been approved. As you mentioned yourself, it will hopefully eliminate unnecessary stack cleanups and maybe point out developer mistakes in case code is placed at the wrong position.
In my opinion, this patch 1/2 is alright. I can split it into two patches if really desired, but personally I don't a reason for it. Patch 2/2 should be revision, so I will add a mail note about it in a few moments. Thanks, Marvin. > -----Original Message----- > From: [email protected] [mailto:[email protected]] > Sent: Sunday, June 12, 2016 7:45 PM > To: Marvin Häuser <[email protected]> > Cc: [email protected]; Gao, Liming <[email protected]> > Subject: Re: [edk2] [PATCH v1 1/2] MdePkg: Add NORETURN attribute and > UNREACHABLE() macro. > > > > 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/Sec > Main.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/Dxe > IplPeim/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
