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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.10.2025 18:01, Dmytro Prokopchuk1 wrote:
> 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
>>>>>>>
>>>>>>>> +     */
>>>>>>>> +
>>>>>>>> +    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).
>>>
>>> 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).
> 
> 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).

Right, because like for ia64 what is being passed around aren't function
pointers, but pointer to the function descriptors.

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

Not sure here.

Jan