subject:"\[edk2\] ASSERT and static code analysis"

Re: [edk2] ASSERT and static code analysis

2016-06-01 Thread Gao, Liming

Laszlo:
  I think this case is same to EFIAPI. For GCC tool chain, 
"-DEFIAPI=__attribute__((ms_abi))" is defined in CC_FLAGS in tools_def.txt. For 
this case, "-DCLANG_ANALYZER_NORETURN=__attribute__((analyzer_noreturn))" can 
only be added into CC_FLAGS for Clang Scan analysis tool chain, like 
CLANGSCAN38 in https://github.com/shijunjing/edk2/tree/llvm. 

Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, June 02, 2016 4:54 AM
> To: Marvin H?user <marvin.haeu...@outlook.com>; edk2-
> de...@lists.01.org <edk2-de...@ml01.01.org>
> Subject: Re: [edk2] ASSERT and static code analysis
> 
> On 06/01/16 22:41, Marvin H?user wrote:
> > Hey Laszlo,
> >
> > Thank you very much for your comment!
> >
> > About the comment in CpuDeadLoop(), once more, I wasn't aware, thanks
> for the hint!
> > Do you think a define should be introduced
> ('NORETURN'/'ANALYZE_NORETURN') to keep the code style consistent and
> also allow support for more static analyzers in the future?
> >
> > If it indeed makes sense, I can get a patch ready, maybe today or tomorrow.
> 
> Honestly, I got no clue how to make this work (in some MdePkg header
> files I guess) across *all* toolchains that edk2 supports. I guess by
> default the macros should expand to an empty replacement text, and the
> "exceptions" should come from toolchain-specific settings in tools_def.txt?
> 
> I'm sure Andrew and the BaseTools maintainers can help.
> 
> Thanks
> Laszlo
> 
> >> -Original Message-
> >> From: Laszlo Ersek [mailto:ler...@redhat.com]
> >> Sent: Wednesday, June 1, 2016 10:38 PM
> >> To: Marvin H?user <marvin.haeu...@outlook.com>
> >> Cc: edk2-devel@lists.01.org <edk2-de...@ml01.01.org>
> >> Subject: Re: [edk2] ASSERT and static code analysis
> >>
> >> On 06/01/16 22:12, Marvin H?user wrote:
> >>> Recently I was told that ASSERT() calls to check whether a variable is
> >>> NULL breaks the Clang Static Analyzer in terms of generating wrong
> >>> warnings. The reason is that, when a variable/parameter is checked for
> >>> NULL, this analyzer assumes that it can be. As it doesn't support
> >>> EDK2 ASSERTs, but only compiler-provided asserts, to it, the ASSERT()
> >>> call is a simple if-check (-> triggers NULL warnings) which does
> >>> return to normal code flow (-> any further usages may be dereferencing
> >>> NULL). This behavior is documented here:
> >>> http://clang-analyzer.llvm.org/faq.html#null_pointer
> >>
> >> Makes sense to me.
> >>
> >>> To make clear that EDK2 ASSERT() calls are indeed asserts, in my
> >>> opinion, CpuDeadLoop() should be flagged as 'noreturn' (it indeed
> >>> should never return) and Breakpoint() as 'analyzer_noreturn' (it may
> >>> return, but the analyzer doesn't have to care as the debugger is
> >>> invoked).
> >>
> >> Side point: CpuDeadLoop() too should be flagged as "analyzer_noreturn"
> >> then; please see the comments in
> >> "MdePkg/Library/BaseLib/CpuDeadLoop.c".
> >>
> >> Thanks
> >> Laszlo
> >>
> >>> If I didn't understand the documentation incorrectly, this should fix
> >>> the issue described in the first paragraph.
> >>>
> >>> If you have experience with the Clang Static Analyzer or even this
> >>> specific issue, I would be happy if you would share your opinion of
> >>> the concern.
> >
> 
> ___
> 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

Re: [edk2] ASSERT and static code analysis

2016-06-01 Thread Shi, Steven

Hi Marvin,

For Clang Static Analyzer, you might look at this edk2 llvm branch.



https://github.com/shijunjing/edk2/tree/llvm



There is a new tool chain dedicated for clang scan-build checkers:

CLANGSCAN38: Base on CLANG38 to seamlessly integrate Clang scan-build analyzer 
infrastructure into edk2 build infrastructure.



* Seamless Integrate Clang scan-build analyzer infrastructure into edk2 
build infrastructure

* Base on CLANG38 to run the clang analyzer command scan-build 
(http://clang-analyzer.llvm.org/ )

* Exactly same as CLANG38 build process, besides run checker on C files

* CLANGSCAN38 = CLANG38 + Run scan-build when compile C files

* See BaseTools\Conf\build_rule.template

[C-Code-File]



"$(SCAN)" $(SCAN_FLAGS) -o $(SCAN_DST) "$(CC)" $(CC_FLAGS) -o ${dst} $(INC) 
${src}

* Build time is longer than CLANG38

* Report output directory is defined in *_CLANGSCAN38_*_SCAN_DST

* Checker selection is defined in *_CLANGSCAN38_*_SCAN_FLAGS



It will be great if you could contribute new Uefi-aware checker based on this 
tool chain. :)





Steven Shi

Intel\SSG\STO\UEFI Firmware



Tel: +86 021-61166522

iNet: 821-6522



> -Original Message-

> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of

> Marvin H?user

> Sent: Thursday, June 02, 2016 5:28 AM

> To: edk2-devel@lists.01.org

> Cc: Laszlo Ersek <ler...@redhat.com>; af...@apple.com

> Subject: Re: [edk2] ASSERT and static code analysis

>

> Hey Andrew,

>

> Thanks for your reply!

>

> In case you haven't read Laszlo's reply, he pointed out the same issue and

> proposed that both CpuDeadLoop() and Breakpoint() should be flagged as '

> analyzer_noreturn', which is ignored by the compiler and thus should not

> break anyone's code, but only suppress the analyzer warnings. :)

>

> Laszlo,

>

> If I understood it correctly, the Clang Static Analyzer is separate from main

> Clang (compiler) and, either way, can be used standalone. For that reason,

> tools_def should probably be avoided as this requires BaseTools invoking the

> analyzer. Instead, one could check for the analyzer define (Clang Static

> Analyzer can be detected via ifdef) and define ANALYZER_NORETURN either

> as empty, when there is no analyzer detected, or as the analyzer-noreturn

> attribute, when a supported analyzer is found. I can get a patch ready by

> tomorrow, I think.

>

> Regards,

> Marvin.

>

> > -Original Message-

> > From: af...@apple.com<mailto:af...@apple.com> [mailto:af...@apple.com]

> > Sent: Wednesday, June 1, 2016 10:56 PM

> > To: Marvin H?user 
> > <marvin.haeu...@outlook.com<mailto:marvin.haeu...@outlook.com>>

> > Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>

> > Subject: Re: [edk2] ASSERT and static code analysis

> >

> >

> > > On Jun 1, 2016, at 1:12 PM, Marvin H?user

> > <marvin.haeu...@outlook.com<mailto:marvin.haeu...@outlook.com>> wrote:

> > >

> > > Recently I was told that ASSERT() calls to check whether a variable is

> > > NULL breaks the Clang Static Analyzer in terms of generating wrong

> > > warnings. The reason is that, when a variable/parameter is checked for

> > > NULL, this analyzer assumes that it can be. As it doesn't support EDK2

> > > ASSERTs, but only compiler-provided asserts, to it, the ASSERT() call

> > > is a simple if-check (-> triggers NULL warnings) which does return to

> > > normal code flow (-> any further usages may be dereferencing NULL).

> > > This behavior is documented here:

> > > http://clang-analyzer.llvm.org/faq.html#null_pointer

> > >

> > > To make clear that EDK2 ASSERT() calls are indeed asserts, in my opinion,

> > CpuDeadLoop() should be flagged as 'noreturn' (it indeed should never

> > return) and Breakpoint() as 'analyzer_noreturn' (it may return, but the

> > analyzer doesn't have to care as the debugger is invoked). If I didn't

> > understand the documentation incorrectly, this should fix the issue

> > described in the first paragraph.

> > >

> >

> > Marvin,

> >

> > Sometimes people use CpuDeadLoop() to debug with a JTAG debugger so

> > they will step over the code. So you can't use noreturn as that tells the

> > optimizer it can remove the code following the no return function. So for

> > example your entire program could get optimized away if you place a

> > CpuBreakpoint() at the start of your function.

> > Simple clang example:

> > ~/work/Compiler>cat noreturn.c

> >

> > int NoReturn(void) __attribute_

Re: [edk2] ASSERT and static code analysis

2016-06-01 Thread Marvin H?user

Hey Andrew,

Thanks for your reply!

In case you haven't read Laszlo's reply, he pointed out the same issue and 
proposed that both CpuDeadLoop() and Breakpoint() should be flagged as ' 
analyzer_noreturn', which is ignored by the compiler and thus should not break 
anyone's code, but only suppress the analyzer warnings. :)

Laszlo,

If I understood it correctly, the Clang Static Analyzer is separate from main 
Clang (compiler) and, either way, can be used standalone. For that reason, 
tools_def should probably be avoided as this requires BaseTools invoking the 
analyzer. Instead, one could check for the analyzer define (Clang Static 
Analyzer can be detected via ifdef) and define ANALYZER_NORETURN either as 
empty, when there is no analyzer detected, or as the analyzer-noreturn 
attribute, when a supported analyzer is found. I can get a patch ready by 
tomorrow, I think.

Regards,
Marvin.

> -Original Message-
> From: af...@apple.com [mailto:af...@apple.com]
> Sent: Wednesday, June 1, 2016 10:56 PM
> To: Marvin H?user <marvin.haeu...@outlook.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] ASSERT and static code analysis
> 
> 
> > On Jun 1, 2016, at 1:12 PM, Marvin H?user
> <marvin.haeu...@outlook.com> wrote:
> >
> > Recently I was told that ASSERT() calls to check whether a variable is
> > NULL breaks the Clang Static Analyzer in terms of generating wrong
> > warnings. The reason is that, when a variable/parameter is checked for
> > NULL, this analyzer assumes that it can be. As it doesn't support EDK2
> > ASSERTs, but only compiler-provided asserts, to it, the ASSERT() call
> > is a simple if-check (-> triggers NULL warnings) which does return to
> > normal code flow (-> any further usages may be dereferencing NULL).
> > This behavior is documented here:
> > http://clang-analyzer.llvm.org/faq.html#null_pointer
> >
> > To make clear that EDK2 ASSERT() calls are indeed asserts, in my opinion,
> CpuDeadLoop() should be flagged as 'noreturn' (it indeed should never
> return) and Breakpoint() as 'analyzer_noreturn' (it may return, but the
> analyzer doesn't have to care as the debugger is invoked). If I didn't
> understand the documentation incorrectly, this should fix the issue
> described in the first paragraph.
> >
> 
> Marvin,
> 
> Sometimes people use CpuDeadLoop() to debug with a JTAG debugger so
> they will step over the code. So you can't use noreturn as that tells the
> optimizer it can remove the code following the no return function. So for
> example your entire program could get optimized away if you place a
> CpuBreakpoint() at the start of your function.
> Simple clang example:
> ~/work/Compiler>cat noreturn.c
> 
> int NoReturn(void) __attribute__ ((noreturn));
> 
> int
> main()
> {
>   NoReturn();
>   return 0;
> }
> ~/work/Compiler>clang -Os -S  noreturn.c ~/work/Compiler>cat noreturn.S
>   .section__TEXT,__text,regular,pure_instructions
>   .macosx_version_min 10, 11
>   .globl  _main
> _main:  ## @main
>   pushq   %rbp
>   movq%rsp, %rbp
>   callq   _NoReturn
> 
> 
> .subsections_via_symbols
> 
> 
> Depending on how much Heisenberg uncertainty you can stand
> 
> You can -D MDEPKG_NDEBUG for your analyzer run.
> https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/D
> ebugLib.h#L288
> 
> In the DSC map the DebugLib library class to
> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseDeb
> ugLibNull/BaseDebugLibNull.inf all the functions are empty and you would
> avoid your issues.
> 
> PcdDebugPropertyMask, set in DSC, can control what happens after an
> ASSERT(), but I'm guessing that is to far into the optimizer to be useful for
> you? If you compiled with clang LTO the CpuBreakpoint() and CpuDeadLoop()
> would get dead stripped.
> https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseDeb
> ugLibSerialPort/DebugLib.c#L146
> 
> Thanks,
> 
> Andrew Fish
> 
> > If you have experience with the Clang Static Analyzer or even this specific
> issue, I would be happy if you would share your opinion of the concern.
> >
> > Regards,
> > Marvin.
> > ___
> > 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

Re: [edk2] ASSERT and static code analysis

2016-06-01 Thread Andrew Fish

> On Jun 1, 2016, at 1:12 PM, Marvin H?user  wrote:
> 
> Recently I was told that ASSERT() calls to check whether a variable is NULL 
> breaks the Clang Static Analyzer in terms of generating wrong warnings. The 
> reason is that, when a variable/parameter is checked for NULL, this analyzer 
> assumes that it can be. As it doesn't support EDK2 ASSERTs, but only 
> compiler-provided asserts, to it, the ASSERT() call is a simple if-check (-> 
> triggers NULL warnings) which does return to normal code flow (-> any further 
> usages may be dereferencing NULL). This behavior is documented here: 
> http://clang-analyzer.llvm.org/faq.html#null_pointer
> 
> To make clear that EDK2 ASSERT() calls are indeed asserts, in my opinion, 
> CpuDeadLoop() should be flagged as 'noreturn' (it indeed should never return) 
> and Breakpoint() as 'analyzer_noreturn' (it may return, but the analyzer 
> doesn't have to care as the debugger is invoked). If I didn't understand the 
> documentation incorrectly, this should fix the issue described in the first 
> paragraph.
> 

Marvin,

Sometimes people use CpuDeadLoop() to debug with a JTAG debugger so they will 
step over the code. So you can't use noreturn as that tells the optimizer it 
can remove the code following the no return function. So for example your 
entire program could get optimized away if you place a CpuBreakpoint() at the 
start of your function. 
Simple clang example:
~/work/Compiler>cat noreturn.c

int NoReturn(void) __attribute__ ((noreturn));

int
main()
{
  NoReturn();
  return 0;
}
~/work/Compiler>clang -Os -S  noreturn.c
~/work/Compiler>cat noreturn.S
.section__TEXT,__text,regular,pure_instructions
.macosx_version_min 10, 11
.globl  _main
_main:  ## @main
pushq   %rbp
movq%rsp, %rbp
callq   _NoReturn

.subsections_via_symbols

Depending on how much Heisenberg uncertainty you can stand

You can -D MDEPKG_NDEBUG for your analyzer run. 
https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Library/DebugLib.h#L288

In the DSC map the DebugLib library class to 
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
 all the functions are empty and you would avoid your issues. 

PcdDebugPropertyMask, set in DSC, can control what happens after an ASSERT(), 
but I'm guessing that is to far into the optimizer to be useful for you? If you 
compiled with clang LTO the CpuBreakpoint() and CpuDeadLoop() would get dead 
stripped. 
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c#L146

Thanks,

Andrew Fish

> If you have experience with the Clang Static Analyzer or even this specific 
> issue, I would be happy if you would share your opinion of the concern.
> 
> Regards,
> Marvin.
> ___
> 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

Re: [edk2] ASSERT and static code analysis

2016-06-01 Thread Laszlo Ersek

On 06/01/16 22:41, Marvin H?user wrote:
> Hey Laszlo,
> 
> Thank you very much for your comment!
> 
> About the comment in CpuDeadLoop(), once more, I wasn't aware, thanks for the 
> hint!
> Do you think a define should be introduced ('NORETURN'/'ANALYZE_NORETURN') to 
> keep the code style consistent and also allow support for more static 
> analyzers in the future?
> 
> If it indeed makes sense, I can get a patch ready, maybe today or tomorrow.

Honestly, I got no clue how to make this work (in some MdePkg header
files I guess) across *all* toolchains that edk2 supports. I guess by
default the macros should expand to an empty replacement text, and the
"exceptions" should come from toolchain-specific settings in tools_def.txt?

I'm sure Andrew and the BaseTools maintainers can help.

Thanks
Laszlo

>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Wednesday, June 1, 2016 10:38 PM
>> To: Marvin H?user <marvin.haeu...@outlook.com>
>> Cc: edk2-devel@lists.01.org <edk2-de...@ml01.01.org>
>> Subject: Re: [edk2] ASSERT and static code analysis
>>
>> On 06/01/16 22:12, Marvin H?user wrote:
>>> Recently I was told that ASSERT() calls to check whether a variable is
>>> NULL breaks the Clang Static Analyzer in terms of generating wrong
>>> warnings. The reason is that, when a variable/parameter is checked for
>>> NULL, this analyzer assumes that it can be. As it doesn't support
>>> EDK2 ASSERTs, but only compiler-provided asserts, to it, the ASSERT()
>>> call is a simple if-check (-> triggers NULL warnings) which does
>>> return to normal code flow (-> any further usages may be dereferencing
>>> NULL). This behavior is documented here:
>>> http://clang-analyzer.llvm.org/faq.html#null_pointer
>>
>> Makes sense to me.
>>
>>> To make clear that EDK2 ASSERT() calls are indeed asserts, in my
>>> opinion, CpuDeadLoop() should be flagged as 'noreturn' (it indeed
>>> should never return) and Breakpoint() as 'analyzer_noreturn' (it may
>>> return, but the analyzer doesn't have to care as the debugger is
>>> invoked).
>>
>> Side point: CpuDeadLoop() too should be flagged as "analyzer_noreturn"
>> then; please see the comments in
>> "MdePkg/Library/BaseLib/CpuDeadLoop.c".
>>
>> Thanks
>> Laszlo
>>
>>> If I didn't understand the documentation incorrectly, this should fix
>>> the issue described in the first paragraph.
>>>
>>> If you have experience with the Clang Static Analyzer or even this
>>> specific issue, I would be happy if you would share your opinion of
>>> the concern.
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] ASSERT and static code analysis

2016-06-01 Thread Marvin H?user

Hey Laszlo,

Thank you very much for your comment!

About the comment in CpuDeadLoop(), once more, I wasn't aware, thanks for the 
hint!
Do you think a define should be introduced ('NORETURN'/'ANALYZE_NORETURN') to 
keep the code style consistent and also allow support for more static analyzers 
in the future?

If it indeed makes sense, I can get a patch ready, maybe today or tomorrow.

Regards,
Marvin.

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, June 1, 2016 10:38 PM
> To: Marvin H?user <marvin.haeu...@outlook.com>
> Cc: edk2-devel@lists.01.org <edk2-de...@ml01.01.org>
> Subject: Re: [edk2] ASSERT and static code analysis
> 
> On 06/01/16 22:12, Marvin H?user wrote:
> > Recently I was told that ASSERT() calls to check whether a variable is
> > NULL breaks the Clang Static Analyzer in terms of generating wrong
> > warnings. The reason is that, when a variable/parameter is checked for
> > NULL, this analyzer assumes that it can be. As it doesn't support
> > EDK2 ASSERTs, but only compiler-provided asserts, to it, the ASSERT()
> > call is a simple if-check (-> triggers NULL warnings) which does
> > return to normal code flow (-> any further usages may be dereferencing
> > NULL). This behavior is documented here:
> > http://clang-analyzer.llvm.org/faq.html#null_pointer
> 
> Makes sense to me.
> 
> > To make clear that EDK2 ASSERT() calls are indeed asserts, in my
> > opinion, CpuDeadLoop() should be flagged as 'noreturn' (it indeed
> > should never return) and Breakpoint() as 'analyzer_noreturn' (it may
> > return, but the analyzer doesn't have to care as the debugger is
> > invoked).
> 
> Side point: CpuDeadLoop() too should be flagged as "analyzer_noreturn"
> then; please see the comments in
> "MdePkg/Library/BaseLib/CpuDeadLoop.c".
> 
> Thanks
> Laszlo
> 
> > If I didn't understand the documentation incorrectly, this should fix
> > the issue described in the first paragraph.
> >
> > If you have experience with the Clang Static Analyzer or even this
> > specific issue, I would be happy if you would share your opinion of
> > the concern.

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] ASSERT and static code analysis

2016-06-01 Thread Laszlo Ersek

On 06/01/16 22:12, Marvin H?user wrote:
> Recently I was told that ASSERT() calls to check whether a variable
> is NULL breaks the Clang Static Analyzer in terms of generating wrong
> warnings. The reason is that, when a variable/parameter is checked
> for NULL, this analyzer assumes that it can be. As it doesn't support
> EDK2 ASSERTs, but only compiler-provided asserts, to it, the ASSERT()
> call is a simple if-check (-> triggers NULL warnings) which does
> return to normal code flow (-> any further usages may be
> dereferencing NULL). This behavior is documented here:
> http://clang-analyzer.llvm.org/faq.html#null_pointer

Makes sense to me.

> To make clear that EDK2 ASSERT() calls are indeed asserts, in my
> opinion, CpuDeadLoop() should be flagged as 'noreturn' (it indeed
> should never return) and Breakpoint() as 'analyzer_noreturn' (it may
> return, but the analyzer doesn't have to care as the debugger is
> invoked).

Side point: CpuDeadLoop() too should be flagged as "analyzer_noreturn"
then; please see the comments in "MdePkg/Library/BaseLib/CpuDeadLoop.c".

Thanks
Laszlo

> If I didn't understand the documentation incorrectly, this
> should fix the issue described in the first paragraph.
> 
> If you have experience with the Clang Static Analyzer or even this
> specific issue, I would be happy if you would share your opinion of
> the concern.

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] ASSERT and static code analysis

Re: [edk2] ASSERT and static code analysis

Re: [edk2] ASSERT and static code analysis

Re: [edk2] ASSERT and static code analysis

Re: [edk2] ASSERT and static code analysis

Re: [edk2] ASSERT and static code analysis

Re: [edk2] ASSERT and static code analysis

7 matches

Site Navigation

Mail list logo

Footer information