Re: [PATCH] misra: add deviation for PrintErrMesg() function

[Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>]

On 8/19/25 18:42, Jan Beulich wrote:
> On 19.08.2025 16:32, Dmytro Prokopchuk1 wrote:
>> On 8/19/25 16:25, Jan Beulich wrote:
>>> On 19.08.2025 15:12, Dmytro Prokopchuk1 wrote:
>>>> MISRA C Rule 2.1 states: "A project shall not contain unreachable code."
>>>>
>>>> The function 'PrintErrMesg()' is implemented to never return control to
>>>> its caller. At the end of its execution, it calls 'blexit()', which, in
>>>> turn, invokes '__builtin_unreachable()'. This makes the 'return false;'
>>>> statement in 'read_file()' function unreachable.
>>>
>>> I'm disappointed. In earlier review comments I pointed out that there are
>>> two. Yet you say "the", without further disambiguation.
>>>
>>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>>> @@ -41,6 +41,10 @@ not executable, and therefore it is safe for them to be 
>>>> unreachable."
>>>>    
>>>> -call_properties+={"name(__builtin_unreachable)&&stmt(begin(any_exp(macro(name(ASSERT_UNREACHABLE)))))",
>>>>  {"noreturn(false)"}}
>>>>    -doc_end
>>>>    
>>>> +-doc_begin="Unreachability caused by the call to the 'PrintErrMesg()' 
>>>> function is deliberate, as it terminates execution, ensuring no control 
>>>> flow continues past this point."
>>>> +-config=MC3A2.R2.1,reports+={deliberate, "any_area(^.*PrintErrMesg.*$ && 
>>>> any_loc(file(^xen/common/efi/boot\\.c$)))"}
>>>> +-doc_end
>>>
>>> I don't understand the description here, nor ...
>>>
>>>> --- a/docs/misra/deviations.rst
>>>> +++ b/docs/misra/deviations.rst
>>>> @@ -97,6 +97,13 @@ Deviations related to MISRA C:2012 Rules:
>>>>           Xen expects developers to ensure code remains safe and reliable 
>>>> in builds,
>>>>           even when debug-only assertions like `ASSERT_UNREACHABLE() are 
>>>> removed.
>>>>    
>>>> +   * - R2.1
>>>> +     - Function `PrintErrMesg()` terminates execution (at the end it calls
>>>> +       `blexit()`, which, in turn, invokes `__builtin_unreachable()`), 
>>>> ensuring
>>>> +       no code beyond this point is ever reached. This guarantees that 
>>>> execution
>>>> +       won't incorrectly proceed or introduce unwanted behavior.
>>>> +     - Tagged as `deliberate` for ECLAIR.
>>>
>>> .. the text here. PrintErrMesg() is noreturn. Why would anything need 
>>> saying about
>>> it? Isn't the problem here solely with the tail of read_file(), while other 
>>> uses
>>> of PrintErrMesg() are okay?
>>
>> I'm a little bit confused.
>>
>> As I understood you proposed to insert the SAF comment before the
>> 'return' statement (with proper justification).
>>
>> And current Eclair configuration & descriptions are not good at all.
> 
> Not sure how that's related, but apart from this, ...
> 
>> Am I right?
> 
> ... yes. Yet how is what you submitted here related to the issue in 
> read_file(),
> which may be addressable by a simple SAF comment (as you say in your reply)?
> 
> Jan

The Eclair reports violation as follows:
"call to function `PrintErrMesg(const CHAR16*, EFI_STATUS)' (unit 
`xen/common/efi/boot.c' with target `xen/arch/arm/efi/boot.o') is one 
cause of unreachability of the next statement"

So, patch was about to ignore violations in file 'xen/common/efi/boot.c'
(actually function read_file() is there) where appears text 'PrintErrMesg'.

Probably this is too unclear. And violation location (read_file()) 
should be explicitly specified...

 From other side simple SAF-xx-safe could address this case as well.

Dmytro.