Re: [RFC PATCH] misra: allow conversion from unsigned long to function pointer

[Dmytro Prokopchuk1 <dmytro_prokopchuk1@xxxxxxxx>]

On 8/14/25 23:43, Nicola Vetrini wrote:
> On 2025-08-14 10:36, Jan Beulich wrote:
>> On 13.08.2025 20:27, Dmytro Prokopchuk1 wrote:
>>> ...
>>>
>>> from `vaddr_t' (that is `unsigned long') to `switch_ttbr_fn*' (that 
>>> is `void(*)(unsigned long)')
>>>
>>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@xxxxxxxx>
>>> ---
>>> This is just a RFC patch.
>>> The commit message is not important at this stage.
>>>
>>> I am seeking comments regarding this case.
>>>
>>> Thanks.
>>> ---
>>>  automation/eclair_analysis/ECLAIR/deviations.ecl |  8 ++++++++
>>>  docs/misra/deviations.rst                        | 10 ++++++++++
>>>  docs/misra/rules.rst                             |  8 +++++++-
>>>  xen/arch/arm/arm64/mmu/mm.c                      |  2 ++
>>>  4 files changed, 27 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/ 
>>> automation/eclair_analysis/ECLAIR/deviations.ecl
>>> index ebce1ceab9..f9fd6076b7 100644
>>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
>>> @@ -365,6 +365,14 @@ constant expressions are required.\""
>>>  }
>>>  -doc_end
>>>
>>> +-doc_begin="The conversion from unsigned long to a function pointer 
>>> does not lose any information, provided that the source type has 
>>> enough bits to restore it."
>>> +-config=MC3A2.R11.1,casts+={safe,
>>> +  "from(type(canonical(builtin(unsigned long))))
>>> +   &&to(type(canonical(__function_pointer_types)))
>>> +   &&relation(definitely_preserves_value)"
>>> +}
>>> +-doc_end
>>> +
> 
> This check is not quite targeted at this situation, as the behaviour of 
> different compilers is a bit of a grey area (even GCC, though that works 
> in practice). The relation is mostly aimed at testing whether the 
> pointer are represented using the same number of bits as unsigned long 
> (which happens to be the case fortunately).

Hi Nicola.

Well, we're telling Eclair the conversion types from() and to(), but can 
Eclair determine their sizes (in bits) for particular architecture?
I mean, is it possible to avoid this "sizeof(unsigned long) == 
sizeof(void (*)())" in source code using only Eclair configs?

Dmytro.

> 
>>>  -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 3c46a1e47a..27848602f6 100644
>>> --- a/docs/misra/deviations.rst
>>> +++ b/docs/misra/deviations.rst
>>> @@ -348,6 +348,16 @@ Deviations related to MISRA C:2012 Rules:
>>>         to store it.
>>>       - Tagged as `safe` for ECLAIR.
>>>
>>> +   * - R11.1
>>> +     - The conversion from unsigned long to a function pointer does 
>>> not lose any
>>> +       information or violate type safety assumptions if the 
>>> unsigned long type
>>> +       is guaranteed to be at least as large as a function pointer. 
>>> This ensures
>>> +       that the function pointer address can be fully represented 
>>> without
>>> +       truncation or corruption. Macro BUILD_BUG_ON can be 
>>> integrated into the
>>> +       build system to confirm that 'sizeof(unsigned long) >= 
>>> sizeof(void (*)())'
>>> +       on all target platforms.
>>
>> If sizeof(unsigned long) > sizeof(void (*)()), there is loss of 
>> information.
>> Unless (not said here) the unsigned long value itself is the result of
>> converting a function pointer to unsigned long. Whether all of that 
>> together
>> can be properly expressed to Eclair I don't know. Hence, as Teddy already
>> suggested, == may want specifying instead.
>>
> 
> +1; it might be worth to add both the eclair config and the 
> BUILD_BUG_ON, noting that neither is sufficient on its own: unless the 
> compiler guarantees not to fiddle with the value is unaltered when cast 
> back and forth all checks on the number of bits are moot.
> 
>>> --- a/xen/arch/arm/arm64/mmu/mm.c
>>> +++ b/xen/arch/arm/arm64/mmu/mm.c
>>> @@ -150,6 +150,7 @@ void __init relocate_and_switch_ttbr(uint64_t ttbr)
>>>      vaddr_t id_addr = virt_to_maddr(relocate_xen);
>>>      relocate_xen_fn *fn = (relocate_xen_fn *)id_addr;
>>>      lpae_t pte;
>>> +    BUILD_BUG_ON(sizeof(unsigned long) < sizeof(fn));
>>>
>>>      /* Enable the identity mapping in the boot page tables */
>>>      update_identity_mapping(true);
>>> @@ -178,6 +179,7 @@ void __init switch_ttbr(uint64_t ttbr)
>>>      vaddr_t id_addr = virt_to_maddr(switch_ttbr_id);
>>>      switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr;
>>>      lpae_t pte;
>>> +    BUILD_BUG_ON(sizeof(unsigned long) < sizeof(fn));
>>>
>>>      /* Enable the identity mapping in the boot page tables */
>>>      update_identity_mapping(true);
>>
>> BUILD_BUG_ON() is a statement, not a declaration, and hence wants 
>> grouping
>> as such. Question is whether we indeed want to sprinkle such checks all
>> over the code base. (I expect the two cases here aren't all we have.)
>>
> 
> +1 as well. I would expect such check to live e.g. in compiler.h or any 
> similarly general header, since this is a widespread and largely arch- 
> neutral property that Xen wants to be always true I believe.
>