Re: [PATCH] Define and use UINT64_C and INT64_C

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.09.2024 15:41, Frediano Ziglio wrote:
> On Mon, Sep 9, 2024 at 11:38 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 09.09.2024 12:08, Frediano Ziglio wrote:
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1384,9 +1384,9 @@ void asmlinkage __init noreturn
>> __start_xen(unsigned long mbi_p)
>>>          }
>>>
>>>          if ( e > min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START,
>>> -                     1UL << (PAGE_SHIFT + 32)) )
>>> +                     UINT64_C(1) << (PAGE_SHIFT + 32)) )
>>>              e = min(HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START,
>>> -                    1UL << (PAGE_SHIFT + 32));
>>> +                    UINT64_C(1) << (PAGE_SHIFT + 32));
>>
>> I disagree - we're dealing with virtual addresses here, which better
>> wouldn't use fixed-width quantities.
>>
> I suppose you are suggesting type-based macros instead of fixed-type
> macros, so something like PADDR_C  and VADDR_C.
> That makes sense.

No, I'm suggesting to somply leave this code alone. On x86 we have no vaddr_t,
and hence I see no reason to have VADDR_C().

>> While not always virtual addresses, I similarly disagree for most or all
>> I've left in context further up: If the underlying type to deal with is
>> unsigned long, constants should match.
>>
> Sure, in this case the underlying type if used as 32 bit cannot be unsigned
> long but they should be unsigned long long (or any 64 bit type).

My primary request here is: Code that won't be built as 32-bit doesn't need
changing if it's not explicitly using {,u}int64_t-type variables /
expressions.

>> --- a/xen/crypto/vmac.c
>>> +++ b/xen/crypto/vmac.c
>>> @@ -11,7 +11,9 @@
>>>  #include <xen/types.h>
>>>  #include <xen/lib.h>
>>>  #include <crypto/vmac.h>
>>> +#ifndef UINT64_C
>>>  #define UINT64_C(x)  x##ULL
>>> +#endif
>>>  /* end for Xen */
>>
>> Here the #define should probably just be dropped?
>>
>>
> If we go for newer type-base macros, we won't need to change here.

I'm afraid I don't understand this reply.

>>> --- a/xen/include/xen/const.h
>>> +++ b/xen/include/xen/const.h
>>> @@ -15,10 +15,19 @@
>>>  #ifdef __ASSEMBLY__
>>>  #define _AC(X,Y)     X
>>>  #define _AT(T,X)     X
>>> +#define UINT64_C(X)     X
>>> +#define INT64_C(X)      X
>>>  #else
>>>  #define __AC(X,Y)    (X##Y)
>>>  #define _AC(X,Y)     __AC(X,Y)
>>>  #define _AT(T,X)     ((T)(X))
>>> +#if __SIZEOF_LONG__ >= 8
>>
>> This is available with gcc 4.3 and newer, yet for now our docs still
>> specify 4.1.2 as the baseline.
>>
> Do we have some sort of configure generated macro for this?

I don#t think we do. And I also don't think we need one - we have
BITS_PER_LONG, which ought to be sufficient here.

>> I'm also unconvinced of the >= - we're talking of fixed-width types here,
>> so imo it needs to be == and then also ...
>>
>>> +#define UINT64_C(X)     X ## UL
>>> +#define INT64_C(X)      X ## L
>>> +#else
>>
>> #elif __SIZEOF_LONG_LONG__ == 8
>>
>> here.
>>
>>> +#define UINT64_C(X)     X ## ULL
>>> +#define INT64_C(X)      X ## LL
>>> +#endif
>>>  #endif
>>
>> Finally if we introduce these, imo we should introduce the other
>> UINT<n>_C()
>> as well, and in a header named after the one mandated by the C library
>> spec.

I'm sorry, I was actually wrong here (alluding to inttypes.h), so ...

>>> --- a/xen/include/xen/stdint.h
>>> +++ b/xen/include/xen/stdint.h
>>> @@ -30,4 +30,6 @@ typedef __UINT64_TYPE__    uint64_t;
>>>
>>>  #endif
>>>
>>> +#include <xen/const.h>
>>
>> Why's this needed?
>>
> Not strictly needed, but in the standard headers they are usually defined
> including stdint.h.

... yes, but imo the definitions then would better live here in the first
place.

Jan