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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2025 01:57, Stefano Stabellini wrote:
> On Tue, 28 Oct 2025, Jan Beulich wrote:
>> On 27.10.2025 19:51, Dmytro Prokopchuk1 wrote:
>>> Rule 11.1 states as following: "Conversions shall not be performed
>>> between a pointer to a function and any other type."
>>>
>>> This deviation from Rule 11.1 relies on both ABI definitions and compiler
>>> implementations supported by Xen. The System V x86_64 ABI and the AArch64
>>> ELF ABI define consistent and compatible representations (i.e., having
>>> the same size and memory layout) for (void *), unsigned long, and function
>>> pointers, enabling safe conversions between these types without data loss
>>> or corruption. Additionally, GCC and Clang, faithfully implement the ABI
>>> specifications, ensuring that the generated machine code conforms to these
>>> guarantees. Developers must note that this behavior is not universal and
>>> depends on platform-specific ABIs and compiler implementations.
>>>
>>> Configure Eclair to avoid reporting violations for conversions from
>>> unsigned long or (void *) to a function pointer.
>>>
>>> Add a compile-time assertion into the file 'xen/common/version.c' to
>>> confirm this conversion compatibility across X86 and ARM platforms
>>> (assuming this file is common for them).
>>>
>>> References:
>>> - System V x86_64 ABI: 
>>> https://gitlab.com/x86-psABIs/x86-64-ABI/-/jobs/artifacts/master/raw/x86-64-ABI/abi.pdf?job=build
>>> - AArch64 ELF ABI: https://github.com/ARM-software/abi-aa/releases
>>> - GCC: https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html
>>> - Clang: https://clang.llvm.org/docs/CrossCompilation.html
>>>
>>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@xxxxxxxx>
>>> Reviewed-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>>> ---
>>> Changes in v4:
>>> - the build assertions for the X86 and ARM are enabled by default 
>>> (unconditionally)
>>> - re-wrote description for the build_assertions() function
>>> - excluded PowerPC architecture (not in scope of certification) from the 
>>> check and wrote apropriate explanation
>>>
>>> Link to v3:
>>> https://patchew.org/Xen/0e72c83102668dfa6f14c4e8f9839b4a73d30b3d.1760458094.git.dmytro._5Fprokopchuk1@xxxxxxxx/
>>> ---
>>>  .../eclair_analysis/ECLAIR/deviations.ecl     |  8 ++++++
>>>  docs/misra/deviations.rst                     |  8 +++++-
>>>  docs/misra/rules.rst                          |  7 +++++-
>>>  xen/common/version.c                          | 25 +++++++++++++++++++
>>>  4 files changed, 46 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl 
>>> b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> index 7f3fd35a33..219ba6993b 100644
>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> @@ -375,6 +375,14 @@ constant expressions are required.\""
>>>  }
>>>  -doc_end
>>>  
>>> +-doc_begin="Conversion from unsigned long or (void *) to a function 
>>> pointer can restore full information, provided that the source type has 
>>> enough bits to restore it."
>>> +-config=MC3A2.R11.1,casts+={safe,
>>> +  "from(type(canonical(builtin(unsigned long)||pointer(builtin(void)))))
>>> +   &&to(type(canonical(__function_pointer_types)))
>>> +   &&relation(definitely_preserves_value)"
>>> +}
>>> +-doc_end
>>> +
>>>  -doc_begin="The conversion from a function pointer to a boolean has a 
>>> well-known semantics that do not lead to unexpected behaviour."
>>>  -config=MC3A2.R11.1,casts+={safe,
>>>    "from(type(canonical(__function_pointer_types)))
>>> diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
>>> index 3271317206..b3431ef24e 100644
>>> --- a/docs/misra/deviations.rst
>>> +++ b/docs/misra/deviations.rst
>>> @@ -366,11 +366,17 @@ Deviations related to MISRA C:2012 Rules:
>>>       - Tagged as `safe` for ECLAIR.
>>>  
>>>     * - R11.1
>>> -     - The conversion from a function pointer to unsigned long or (void 
>>> \*) does
>>> +     - The conversion from a function pointer to unsigned long or '(void 
>>> *)' does
>>>         not lose any information, provided that the target type has enough 
>>> bits
>>>         to store it.
>>>       - Tagged as `safe` for ECLAIR.
>>>  
>>> +   * - R11.1
>>> +     - Conversion from unsigned long or '(void *)' to a function pointer 
>>> can
>>> +       restore full information, provided that the source type has enough 
>>> bits
>>> +       to restore it.
>>> +     - Tagged as `safe` for ECLAIR.
>>> +
>>>     * - R11.1
>>>       - The conversion from a function pointer to a boolean has a well-known
>>>         semantics that do not lead to unexpected behaviour.
>>> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
>>> index 4388010ec9..4e94251887 100644
>>> --- a/docs/misra/rules.rst
>>> +++ b/docs/misra/rules.rst
>>> @@ -431,7 +431,12 @@ maintainers if you want to suggest a change.
>>>       - All conversions to integer types are permitted if the destination
>>>         type has enough bits to hold the entire value. Conversions to bool
>>>         and void* are permitted. Conversions from 'void noreturn (*)(...)'
>>> -       to 'void (*)(...)' are permitted.
>>> +       to 'void (*)(...)' are permitted. Conversions from unsigned long or
>>> +       '(void *)' to a function pointer are permitted.
>>> +       Example::
>>> +
>>> +           unsigned long func_addr = (unsigned long)&some_function;
>>> +           void (*restored_func)(void) = (void (*)(void))func_addr;
>>>  
>>>     * - `Rule 11.2 
>>> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_11_02.c>`_
>>>       - Required
>>> diff --git a/xen/common/version.c b/xen/common/version.c
>>> index 553b97ba9b..674bd72b31 100644
>>> --- a/xen/common/version.c
>>> +++ b/xen/common/version.c
>>> @@ -217,6 +217,31 @@ void __init xen_build_init(void)
>>>  #endif /* CONFIG_X86 */
>>>  }
>>>  #endif /* BUILD_ID */
>>> +
>>> +/*
>>> + * 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:
>>> +     * - PowerPC: uses function descriptors (code address + TOC pointer).
>>> +     */
>>
>> Yes, it uses function descriptors (aiui they consist of three longs, not just
>> two though), but "function pointers are larger than data pointers" is still
>> wrong there, which is why (as you indicated before) ...
>>
>>> +#if !defined(__powerpc__)
>>> +    BUILD_BUG_ON(sizeof(unsigned long) != sizeof(void (*)(void)));
>>> +    BUILD_BUG_ON(sizeof(void *) != sizeof(void (*)(void)));
>>> +#endif
>>
>> ... these checks still will not cause build breakage even if enabled for PPC.
>> At least aiui (what is being passed around are pointers to function
>> descriptors). IOW I don't view it as justified to exclude PPC here, at least
>> not yet.
> 
> Could the patch be committed without the #if !defined(__powerpc__) /
> #endif chunk considering that according to Dmytro it should pass the
> pipeline anyway?

And with the comment also dropped (or adjusted accordingly).

Jan