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

[Jan Beulich <jbeulich@xxxxxxxx>]

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