multiboot2 structs and packed attribute

multiboot2 structs and packed attribute

[Daniel Kiper]

Hi,

Does anybody know why only multiboot2 multiboot_mmap_entry struct
has packed attribute? Is it by design or by mistake? I think that
we should use packed attribute for every multiboot2 protocol related
struct. If not then we should probably remove this attribute from
multiboot_mmap_entry struct too. What do you think about that?

Daniel

Re: multiboot2 structs and packed attribute

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

Le lun. 7 mars 2016 14:35, Daniel Kiper <daniel.kiper@oracle.com> a écrit :

> Hi,
>
> Does anybody know why only multiboot2 multiboot_mmap_entry struct
> has packed attribute? Is it by design or by mistake? I think that
> we should use packed attribute for every multiboot2 protocol related
> struct. If not then we should probably remove this attribute from
> multiboot_mmap_entry struct too. What do you think about that?
>
We put packed only when it changes relative offsets of members or size of
the structure. Is it the case for structures you have in mind?

>
> Daniel
>

[-- Attachment #2: Type: text/html, Size: 990 bytes --]

Re: multiboot2 structs and packed attribute

[Daniel Kiper]

On Mon, Mar 07, 2016 at 08:42:38PM +0000, Vladimir 'phcoder' Serbinenko wrote:
> Le lun. 7 mars 2016 14:35, Daniel Kiper <daniel.kiper@oracle.com> a écrit :
>
> > Hi,
> >
> > Does anybody know why only multiboot2 multiboot_mmap_entry struct
> > has packed attribute? Is it by design or by mistake? I think that
> > we should use packed attribute for every multiboot2 protocol related
> > struct. If not then we should probably remove this attribute from
> > multiboot_mmap_entry struct too. What do you think about that?
> >
> We put packed only when it changes relative offsets of members or size of
> the structure. Is it the case for structures you have in mind?

I do not have anything specific. However, it looks like stray stuff.

Hmmm... I took a look at multiboot_mmap_entry once again and it seems
that this struct really does not need packed attribute. All members are
in right order and even compiler should not add padding. Am I right?

Anyway, maybe we should add somewhere in the header comment saying that
all/most of multiboot2 structures are build in a way that fulfill alignment
requirements out of the box and compiler does not need to do any rearrangements.
However, we should warn that new extensions should be added carefully and
some structs may require GRUB_PACKED attribute in the future to properly
represent added interface.

Or we can add GRUB_PACKED to every struct. However, this seems redundant here.
There is no chance that somebody will rearrange, especially blindly, existing structs.

Daniel

Re: multiboot2 structs and packed attribute

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1864 bytes --]

On Tuesday, March 8, 2016, Daniel Kiper <daniel.kiper@oracle.com> wrote:

> On Mon, Mar 07, 2016 at 08:42:38PM +0000, Vladimir 'phcoder' Serbinenko
> wrote:
> > Le lun. 7 mars 2016 14:35, Daniel Kiper <daniel.kiper@oracle.com
> <javascript:;>> a écrit :
> >
> > > Hi,
> > >
> > > Does anybody know why only multiboot2 multiboot_mmap_entry struct
> > > has packed attribute? Is it by design or by mistake? I think that
> > > we should use packed attribute for every multiboot2 protocol related
> > > struct. If not then we should probably remove this attribute from
> > > multiboot_mmap_entry struct too. What do you think about that?
> > >
> > We put packed only when it changes relative offsets of members or size of
> > the structure. Is it the case for structures you have in mind?
>
> I do not have anything specific. However, it looks like stray stuff.
>
> Hmmm... I took a look at multiboot_mmap_entry once again and it seems
> that this struct really does not need packed attribute. All members are
> in right order and even compiler should not add padding. Am I right?
>
Yes, good catch.

>
> Anyway, maybe we should add somewhere in the header comment saying that
> all/most of multiboot2 structures are build in a way that fulfill alignment
> requirements out of the box and compiler does not need to do any
> rearrangements.
> However, we should warn that new extensions should be added carefully and
> some structs may require GRUB_PACKED attribute in the future to properly
> represent added interface.
>
This probably should go to spec rather than the header file.

>
> Or we can add GRUB_PACKED to every struct. However, this seems redundant
> here.
> There is no chance that somebody will rearrange, especially blindly,
> existing structs.
>
> Daniel
>


-- 
Regards
Vladimir 'phcoder' Serbinenko

[-- Attachment #2: Type: text/html, Size: 2508 bytes --]