Re: [PATCH 3/3] tools: add offsetof() to xen-tools/libs.h

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/02/2023 2:12 pm, Jan Beulich wrote:
> On 27.02.2023 15:06, Andrew Cooper wrote:
>> On 27/02/2023 12:43 pm, Jan Beulich wrote:
>>> On 27.02.2023 13:34, Juergen Gross wrote:
>>>> On 27.02.23 13:31, Jan Beulich wrote:
>>>>> On 27.02.2023 13:09, Juergen Gross wrote:
>>>>>> --- a/tools/firmware/hvmloader/util.h
>>>>>> +++ b/tools/firmware/hvmloader/util.h
>>>>>> @@ -30,9 +30,6 @@ enum {
>>>>>>   #define SEL_DATA32          0x0020
>>>>>>   #define SEL_CODE64          0x0028
>>>>>>   
>>>>>> -#undef offsetof
>>>>>> -#define offsetof(t, m) ((unsigned long)&((t *)0)->m)
>>>>>> -
>>>>>>   #undef NULL
>>>>>>   #define NULL ((void*)0)
>>>>>>   
>>>>>> diff --git a/tools/firmware/include/stddef.h 
>>>>>> b/tools/firmware/include/stddef.h
>>>>>> index c7f974608a..926ae12fa5 100644
>>>>>> --- a/tools/firmware/include/stddef.h
>>>>>> +++ b/tools/firmware/include/stddef.h
>>>>>> @@ -1,10 +1,10 @@
>>>>>>   #ifndef _STDDEF_H_
>>>>>>   #define _STDDEF_H_
>>>>>>   
>>>>>> +#include <xen-tools/libs.h>
>>>>> I'm not convinced firmware/ files can validly include this header.
>>>> This file only contains macros which don't call any external functions.
>>>>
>>>> Would you be fine with me adding a related comment to it?
>>> If it was to be usable like this, that would be a prereq, but personally
>>> I don't view this as sufficient. The environment code runs in that lives
>>> under firmware/ is simply different (hvmloader, for example, is 32-bit
>>> no matter that the toolstack would normally be 64-bit), so to me it
>>> feels like setting up a trap for ourselves. If Andrew or Roger are fully
>>> convinced this is a good move, I won't be standing in the way ...
>> We still support 32bit builds of the toolstack on multiple
>> architectures, so I don't see bitness as an argument against.
> Bitness by itself wasn't the argument. Mixed bitness is what concerns
> me.

I still don't see how that would matter.  The bitness would always be
consistent inside a single translation.

>
>> I am slightly uneasy adding a header like this, but it really is only
>> common macros and I don't see how it could possibly change from that
>> given the existing uses.  OTOH, removing things like the problematic
>> definition of offsetof() is an improvement.
>>
>> I'd probably tentatively ack this patch, seeing as it is a minor
>> improvlement, but I think there's a middle option too.  Rename libs.h to
>> macros.h or common-macros.h?  IMO that would be something that's far
>> more obviously safe to include into firmware/, and something rather more
>> descriptive at all of its include sites.
>>
>> Thoughts?
> Maybe. One fundamental requirement would need to be made prominently
> visible in such a header: It has to be entirely self-contained, i.e.
> it may in particular not gain any dependencies on configure's output.

That's a reasonable restriction IMO.  Again, I don't see how we'd ever
come to violate that in the future, given the current use here.

~Andrew