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:[email protected]]
>> Sent: Wednesday, June 1, 2016 10:38 PM
>> To: Marvin H?user <[email protected]>
>> Cc: [email protected] <[email protected]>
>> 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
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
