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
