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

[Jan Beulich <jbeulich@xxxxxxxx>]

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 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.

Jan