| Message ID | 20240430110922.15052-2-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Fix MISRA regression about flexible array member not at the end | expand |
On 30.04.2024 13:09, Luca Fancellu wrote: > --- a/xen/include/xen/kernel.h > +++ b/xen/include/xen/kernel.h > @@ -54,6 +54,27 @@ > typeof_field(type, member) *__mptr = (ptr); \ > (type *)( (char *)__mptr - offsetof(type,member) );}) > > +/** > + * __struct_group() - Create a mirrored named and anonyomous struct > + * > + * @TAG: The tag name for the named sub-struct (usually empty) > + * @NAME: The identifier name of the mirrored sub-struct > + * @ATTRS: Any struct attributes (usually empty) > + * @MEMBERS: The member declarations for the mirrored structs > + * > + * Used to create an anonymous union of two structs with identical layout > + * and size: one anonymous and one named. The former's members can be used > + * normally without sub-struct naming, and the latter can be used to > + * reason about the start, end, and size of the group of struct members. > + * The named struct can also be explicitly tagged for layer reuse, as well > + * as both having struct attributes appended. > + */ > +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \ > + union { \ > + struct { MEMBERS } ATTRS; \ > + struct TAG { MEMBERS } ATTRS NAME; \ > + } ATTRS Besides my hesitance towards having this construct, can you explain why ATTR needs using 3 times, i.e. also on the wrapping union? Jan
Hi Jan, > On 30 Apr 2024, at 12:43, Jan Beulich <jbeulich@suse.com> wrote: > > On 30.04.2024 13:09, Luca Fancellu wrote: >> --- a/xen/include/xen/kernel.h >> +++ b/xen/include/xen/kernel.h >> @@ -54,6 +54,27 @@ >> typeof_field(type, member) *__mptr = (ptr); \ >> (type *)( (char *)__mptr - offsetof(type,member) );}) >> >> +/** >> + * __struct_group() - Create a mirrored named and anonyomous struct >> + * >> + * @TAG: The tag name for the named sub-struct (usually empty) >> + * @NAME: The identifier name of the mirrored sub-struct >> + * @ATTRS: Any struct attributes (usually empty) >> + * @MEMBERS: The member declarations for the mirrored structs >> + * >> + * Used to create an anonymous union of two structs with identical layout >> + * and size: one anonymous and one named. The former's members can be used >> + * normally without sub-struct naming, and the latter can be used to >> + * reason about the start, end, and size of the group of struct members. >> + * The named struct can also be explicitly tagged for layer reuse, as well >> + * as both having struct attributes appended. >> + */ >> +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \ >> + union { \ >> + struct { MEMBERS } ATTRS; \ >> + struct TAG { MEMBERS } ATTRS NAME; \ >> + } ATTRS > > Besides my hesitance towards having this construct, can you explain why > ATTR needs using 3 times, i.e. also on the wrapping union? The original commit didn’t have the third ATTRS, but afterwards it was introduced due to this: https://patchwork.kernel.org/project/linux-wireless/patch/20231120110607.98956-1-dmantipov@yandex.ru/#25610045 Now, I have to say that for the Origin tag I used the SHA of the commit introducing the macro and the SHA doing this modification is different, how are these cases handled? Cheers, Luca
On 01.05.2024 08:54, Luca Fancellu wrote: >> On 30 Apr 2024, at 12:43, Jan Beulich <jbeulich@suse.com> wrote: >> On 30.04.2024 13:09, Luca Fancellu wrote: >>> --- a/xen/include/xen/kernel.h >>> +++ b/xen/include/xen/kernel.h >>> @@ -54,6 +54,27 @@ >>> typeof_field(type, member) *__mptr = (ptr); \ >>> (type *)( (char *)__mptr - offsetof(type,member) );}) >>> >>> +/** >>> + * __struct_group() - Create a mirrored named and anonyomous struct >>> + * >>> + * @TAG: The tag name for the named sub-struct (usually empty) >>> + * @NAME: The identifier name of the mirrored sub-struct >>> + * @ATTRS: Any struct attributes (usually empty) >>> + * @MEMBERS: The member declarations for the mirrored structs >>> + * >>> + * Used to create an anonymous union of two structs with identical layout >>> + * and size: one anonymous and one named. The former's members can be used >>> + * normally without sub-struct naming, and the latter can be used to >>> + * reason about the start, end, and size of the group of struct members. >>> + * The named struct can also be explicitly tagged for layer reuse, as well >>> + * as both having struct attributes appended. >>> + */ >>> +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \ >>> + union { \ >>> + struct { MEMBERS } ATTRS; \ >>> + struct TAG { MEMBERS } ATTRS NAME; \ >>> + } ATTRS >> >> Besides my hesitance towards having this construct, can you explain why >> ATTR needs using 3 times, i.e. also on the wrapping union? > > The original commit didn’t have the third ATTRS, but afterwards it was introduced due to > this: > > https://patchwork.kernel.org/project/linux-wireless/patch/20231120110607.98956-1-dmantipov@yandex.ru/#25610045 Hmm. Since generally packing propagates to containing compound types, I'd say this cannot go without a code comment, or at the very least a mention in the description. But: Do we use this old ABI in Xen at all? IOW can we get away without this 3rd instance? > Now, I have to say that for the Origin tag I used the SHA of the commit introducing the macro > and the SHA doing this modification is different, how are these cases handled? I'd say the hash of the original commit is enough even if the 3rd instance needs keeping for whatever reason. Jan
> On 2 May 2024, at 07:09, Jan Beulich <jbeulich@suse.com> wrote: > > On 01.05.2024 08:54, Luca Fancellu wrote: >>> On 30 Apr 2024, at 12:43, Jan Beulich <jbeulich@suse.com> wrote: >>> On 30.04.2024 13:09, Luca Fancellu wrote: >>>> --- a/xen/include/xen/kernel.h >>>> +++ b/xen/include/xen/kernel.h >>>> @@ -54,6 +54,27 @@ >>>> typeof_field(type, member) *__mptr = (ptr); \ >>>> (type *)( (char *)__mptr - offsetof(type,member) );}) >>>> >>>> +/** >>>> + * __struct_group() - Create a mirrored named and anonyomous struct >>>> + * >>>> + * @TAG: The tag name for the named sub-struct (usually empty) >>>> + * @NAME: The identifier name of the mirrored sub-struct >>>> + * @ATTRS: Any struct attributes (usually empty) >>>> + * @MEMBERS: The member declarations for the mirrored structs >>>> + * >>>> + * Used to create an anonymous union of two structs with identical layout >>>> + * and size: one anonymous and one named. The former's members can be used >>>> + * normally without sub-struct naming, and the latter can be used to >>>> + * reason about the start, end, and size of the group of struct members. >>>> + * The named struct can also be explicitly tagged for layer reuse, as well >>>> + * as both having struct attributes appended. >>>> + */ >>>> +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \ >>>> + union { \ >>>> + struct { MEMBERS } ATTRS; \ >>>> + struct TAG { MEMBERS } ATTRS NAME; \ >>>> + } ATTRS >>> >>> Besides my hesitance towards having this construct, can you explain why >>> ATTR needs using 3 times, i.e. also on the wrapping union? >> >> The original commit didn’t have the third ATTRS, but afterwards it was introduced due to >> this: >> >> https://patchwork.kernel.org/project/linux-wireless/patch/20231120110607.98956-1-dmantipov@yandex.ru/#25610045 > > Hmm. Since generally packing propagates to containing compound types, I'd > say this cannot go without a code comment, or at the very least a mention > in the description. But: Do we use this old ABI in Xen at all? IOW can we > get away without this 3rd instance? Yes, I think it won’t be a problem for Xen, is it something that can be done on commit? Anyway in case of comments on the second patch, I’ll push the change also for this one. Cheers, Luca
On 02.05.2024 08:23, Luca Fancellu wrote: > > >> On 2 May 2024, at 07:09, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 01.05.2024 08:54, Luca Fancellu wrote: >>>> On 30 Apr 2024, at 12:43, Jan Beulich <jbeulich@suse.com> wrote: >>>> On 30.04.2024 13:09, Luca Fancellu wrote: >>>>> --- a/xen/include/xen/kernel.h >>>>> +++ b/xen/include/xen/kernel.h >>>>> @@ -54,6 +54,27 @@ >>>>> typeof_field(type, member) *__mptr = (ptr); \ >>>>> (type *)( (char *)__mptr - offsetof(type,member) );}) >>>>> >>>>> +/** >>>>> + * __struct_group() - Create a mirrored named and anonyomous struct >>>>> + * >>>>> + * @TAG: The tag name for the named sub-struct (usually empty) >>>>> + * @NAME: The identifier name of the mirrored sub-struct >>>>> + * @ATTRS: Any struct attributes (usually empty) >>>>> + * @MEMBERS: The member declarations for the mirrored structs >>>>> + * >>>>> + * Used to create an anonymous union of two structs with identical layout >>>>> + * and size: one anonymous and one named. The former's members can be used >>>>> + * normally without sub-struct naming, and the latter can be used to >>>>> + * reason about the start, end, and size of the group of struct members. >>>>> + * The named struct can also be explicitly tagged for layer reuse, as well >>>>> + * as both having struct attributes appended. >>>>> + */ >>>>> +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \ >>>>> + union { \ >>>>> + struct { MEMBERS } ATTRS; \ >>>>> + struct TAG { MEMBERS } ATTRS NAME; \ >>>>> + } ATTRS >>>> >>>> Besides my hesitance towards having this construct, can you explain why >>>> ATTR needs using 3 times, i.e. also on the wrapping union? >>> >>> The original commit didn’t have the third ATTRS, but afterwards it was introduced due to >>> this: >>> >>> https://patchwork.kernel.org/project/linux-wireless/patch/20231120110607.98956-1-dmantipov@yandex.ru/#25610045 >> >> Hmm. Since generally packing propagates to containing compound types, I'd >> say this cannot go without a code comment, or at the very least a mention >> in the description. But: Do we use this old ABI in Xen at all? IOW can we >> get away without this 3rd instance? > > Yes, I think it won’t be a problem for Xen, is it something that can be done on commit? Don't know, maybe. First you need an ack, and I remain unconvinced that we actually need this construct. Jan > Anyway in case of comments on the second patch, I’ll push the change also for this one. > > Cheers, > Luca >
On Tue, 30 Apr 2024, Luca Fancellu wrote: > Import __struct_group from Linux, commit 50d7bd38c3aa > ("stddef: Introduce struct_group() helper macro"), in order to > allow the access through the anonymous structure to the members > without having to write also the name, e.g: > > struct foo { > int one; > struct { > int two; > int three, four; > } thing; > int five; > }; > > would become: > > struct foo { > int one; > __struct_group(/* None */, thing, /* None */, > int two; > int three, four; > ); > int five; > }; > > Allowing the users of this structure to access the .thing members by > using .two/.three/.four on the struct foo. > This construct will become useful in order to have some generalized > interfaces that shares some common members. > > Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 50d7bd38c3aa > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h index bb6b0f38912d..bc2440b5f96e 100644 --- a/xen/include/xen/kernel.h +++ b/xen/include/xen/kernel.h @@ -54,6 +54,27 @@ typeof_field(type, member) *__mptr = (ptr); \ (type *)( (char *)__mptr - offsetof(type,member) );}) +/** + * __struct_group() - Create a mirrored named and anonyomous struct + * + * @TAG: The tag name for the named sub-struct (usually empty) + * @NAME: The identifier name of the mirrored sub-struct + * @ATTRS: Any struct attributes (usually empty) + * @MEMBERS: The member declarations for the mirrored structs + * + * Used to create an anonymous union of two structs with identical layout + * and size: one anonymous and one named. The former's members can be used + * normally without sub-struct naming, and the latter can be used to + * reason about the start, end, and size of the group of struct members. + * The named struct can also be explicitly tagged for layer reuse, as well + * as both having struct attributes appended. + */ +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \ + union { \ + struct { MEMBERS } ATTRS; \ + struct TAG { MEMBERS } ATTRS NAME; \ + } ATTRS + /* * Check at compile time that something is of a particular type. * Always evaluates to 1 so you may use it easily in comparisons.
Import __struct_group from Linux, commit 50d7bd38c3aa ("stddef: Introduce struct_group() helper macro"), in order to allow the access through the anonymous structure to the members without having to write also the name, e.g: struct foo { int one; struct { int two; int three, four; } thing; int five; }; would become: struct foo { int one; __struct_group(/* None */, thing, /* None */, int two; int three, four; ); int five; }; Allowing the users of this structure to access the .thing members by using .two/.three/.four on the struct foo. This construct will become useful in order to have some generalized interfaces that shares some common members. Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 50d7bd38c3aa Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/include/xen/kernel.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)