Re: [PATCH 05/12] x86/crash: address Misra 2.1 rule violation

[Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>]

On 9/14/26 09:44, Jan Beulich wrote:
> On 11.09.2026 20:53, Nicola Vetrini wrote:
>> On 2026-09-10 14:29, Roger Pau Monné wrote:
>>> On Thu, Sep 10, 2026 at 11:52:10AM +0200, Jan Beulich wrote:
>>>> On 10.09.2026 11:30, Roger Pau Monné wrote:
>>>>> On Thu, Sep 10, 2026 at 10:38:34AM +0200, Jan Beulich wrote:
>>>>>> On 10.09.2026 09:49, Roger Pau Monné wrote:
>>>>>>> On Fri, Aug 28, 2026 at 09:02:18AM +0200, Jan Beulich wrote:
>>>>>>>> The use of unreachable(), when unreachability is visible to Eclair (and
>>>>>>>> compilers), is deemed a violation. Drop the redundant statement.
>>>>>>>
>>>>>>> Urg, isn't that something that should be fixed in Eclair then?
>>>>>>> Otherwise all the unreachable() calls in our codebase are likely to be
>>>>>>> found by Eclair sooner or later, and will need to be removed.
>>>>>>
>>>>>> No, aiui most are covered by deviations. In particular ones in BUG() and
>>>>>> ASSERT_UNREACHABLE().
>>>>>
>>>>> Shouldn't this be a deviation then also?
>>>>
>>>> Maybe, just that I had no good idea how to express such a deviation 
>>>> (preferably
>>>> without a SAF comment).
>>>>
>>>>>    Maybe it would be helpful if
>>>>> the commit message states why this is handled differently from other
>>>>> unreachable() instances then.
>>>>
>>>> I've added "..., , and the one here isn't covered by a deviation" to the 
>>>> first
>>>> sentence. Will that suffice?
>>>
>>> TBH, the handling of unreachable() feels inconsistent to me.  I don't
>>> blame you for this, I know you are just trying to fix the remaining
>>> issues.
>>>
>>> I guess I will defer the change to someone more familiar with MISRA
>>> and why some unreachable() usages are covered by deviations while
>>> others aren't.
>>>
>>> I think the point of adding something to the commit message is to
>>> justify why this is removed vs a deviation being added.
>>
>> Actually this should be done with a deviation, and I thought it was already 
>> taken care of by
>>
>> -config=MC3A2.R2.1,statements+={deliberate, 
>> "call(decl(name(__builtin_unreachable||panic||do_unexpected_trap||machine_halt||machine_restart||reboot_or_halt)))"}
>>
>> namely because unreachable() expands to a call to __builtin_unreachable(). 
>> It might be worth checking why that is not the case. Probably the 
>> configuration needs a slight tweaking.
> 
> Question (on my side at least) is: How would such "checking" look like?
> As said elsewhere, the syntax used in and the semantics of all these
> deviations are close to impossible to fully understand (which is very
> certainly different for you). Yet without fully understanding what's
> written at present, how would one "check" that this is actually what is
> wanted/needed?
> 
> Jan

In this particular case the violation is caused by the 
__builtin_unreachable() itself, because it is located after for(;;), 
according to the Eclair's report:

xen/arch/x86/crash.c:119.5-119.7: `for' statement is one cause of 
unreachability of the next statement
xen/arch/x86/crash.c:122.5-122.17: reference to function 
`__builtin_unreachable(void)' is unreachable
<preprocessed xen/arch/x86/crash.c>:16007.5-16007.25: preprocessed tokens
xen/include/xen/compiler.h:50.23-50.43: expanded from macro `unreachable'

That deviation "-config=MC3A2.R2.1,statements+={deliberate, 
"call(decl(name(__builtin_unreachable ..." is about code placed after 
__builtin_unreachable() (I hope I'm right).

Actually, Eclair says us that we do not need to place (unnecessary) 
__builtin_unreachable() after for(;;).

 From my point of view, Jan's patch is OK. Maybe just need to mention 
this loop for(;;) in commit message.

Dmytro.