Re: [PATCH v3] misra: consider conversion from UL or (void*) to function pointer as safe

[Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>]

On 10/23/25 17:41, Jan Beulich wrote:
> On 23.10.2025 15:57, Dmytro Prokopchuk1 wrote:
>>
>>
>> On 10/23/25 13:23, Jan Beulich wrote:
>>> On 23.10.2025 12:00, Dmytro Prokopchuk1 wrote:
>>>> On 10/17/25 10:09, Nicola Vetrini wrote:
>>>>> On 2025-10-15 08:20, Jan Beulich wrote:
>>>>>> On 14.10.2025 18:16, Dmytro Prokopchuk1 wrote:
>>>>>>> --- a/xen/common/version.c
>>>>>>> +++ b/xen/common/version.c
>>>>>>> @@ -217,6 +217,20 @@ void __init xen_build_init(void)
>>>>>>>    #endif /* CONFIG_X86 */
>>>>>>>    }
>>>>>>>    #endif /* BUILD_ID */
>>>>>>> +
>>>>>>> +#if defined(__i386__) || defined(__x86_64__) || defined(__arm__) ||
>>>>>>> defined(__aarch64__)
>>>>>>
>>>>>> Why __i386__? Also (nit): Line too long.
>>>>
>>>> Well, I copied this line from Xen codebase,
>>>> but yeah, __i386__ is outdated now.
>>>> I'll remove it.
>>>>
>>>>>>
>>>>>> And why this restriction without any comment here or ...
>>>>>>
>>>>>>> +static void __init __maybe_unused build_assertions(void)
>>>>>>> +{
>>>>>>> +    /*
>>>>>>> +     * To confirm conversion compatibility between unsigned long,
>>>>>>> (void *)
>>>>>>> +     * and function pointers for X86 and ARM architectures only.
>>>>>>
>>>>>> ... explanation here? More generally - how would people know to update
>>>>>> the condition if another port was to be certified?
>>>>>>
>>>>>> Finally, with the v3 addition here, is Nicola's R-b really still
>>>>>> applicable?
>>>>>>
>>>>>
>>>>> I agree with the point you make about i386 (e.g., C-language-
>>>>> toolchain.rst may be mentioned to provide some context about the
>>>>> preprocessor guard); that said, my R-by can be retained
>>>>>
>>>>>> Jan
>>>>>>
>>>>>>> +     */
>>>>>>> +
>>>>>>> +    BUILD_BUG_ON(sizeof(unsigned long) != sizeof(void (*)(void)));
>>>>>>> +    BUILD_BUG_ON(sizeof(void *) != sizeof(void (*)(void)));
>>>>>>> +}
>>>>>>> +#endif
>>>>>>> +
>>>>>>>    /*
>>>>>>>     * Local variables:
>>>>>>>     * mode: C
>>>>>
>>>>
>>>> And probably v4 can have the following wording:
>>>>
>>>> /*
>>>>     * This assertion checks compatibility between 'unsigned long', 'void 
>>>> *',
>>>>     * and function pointers. This is true for X86 (x86_64) and ARM (arm,
>>>> aarch64)
>>>>     * architectures, which is why the check is restricted to these.
>>>>     *
>>>>     * For more context on architecture-specific preprocessor guards, see
>>>>     * docs/misc/C-language-toolchain.rst.
>>>>     *
>>>>     * If Xen is ported to a new architecture, verify that this
>>>> compatibility holds
>>>>     * before adding its macro to the condition below. If the compatibility
>>>> does not
>>>>     * hold, this assertion may need to be revised or removed for that
>>>> architecture.
>>>>     */
>>>
>>> Except that this doesn't address my concern. Imo the checks want to be there
>>> unconditionally, and ports where they're _not_ applicable would then need
>>> excluding (with suitable commentary and/or alternative checks).
>>>
>>> Jan
>>
>> Ok, below is the updated logic:
>>
>> /*
>>    * This assertion checks compatibility between 'unsigned long', 'void *',
>>    * and function pointers. This is true for most supported architectures,
>>    * including X86 (x86_64) and ARM (arm, aarch64).
>>    *
>>    * For more context on architecture-specific preprocessor guards, see
>>    * docs/misc/C-language-toolchain.rst.
>>    *
>>    * If porting Xen to a new architecture where this compatibility does
>> not hold,
>>    * exclude that architecture from these checks and provide suitable
>> commentary
>>    * and/or alternative checks as appropriate.
>>    */
>> static void __init __maybe_unused build_assertions(void)
>> {
>>       /*
>>        * Exclude architectures where function pointers are larger than
>> data pointers:
>>        * - IA-64: uses 'fat' function pointers (code address + global
>> pointer)
>>        */
>> #if !defined(__ia64__)
>>       BUILD_BUG_ON(sizeof(unsigned long) != sizeof(void (*)(void)));
>>       BUILD_BUG_ON(sizeof(void *) != sizeof(void (*)(void)));
>> #endif
>> }
> 
> I would omit architectures we don't support, though. I gave IA-64 as an
> example where things are more complicated (albeit iirc the checks would still
> succeed there). However, I didn't expect any trace of it to be added to the
> code base (again).
> 
> Jan

Well, looks like only __powerpc__ matches these criterias.
At least, I see it in 'xen/arch'.

But, this assertion didn't trigger build to fail, when I run CI:
https://gitlab.com/xen-project/people/dimaprkp4k/xen/-/jobs/11822940884
because PPC64 pointer size is 64-bits (according to the 
C-language-toolchain.rst).

In any case the __powerpc__ is out of scope of certification, so this 
architecture should be excluded.

Dmytro.