Re: [PATCH] Fix packing issue of machine_mmap_entry

[Christian Franke]

Marco Gerards wrote:

> > ...
> >         Add compile time assert to check packing.
> >     
>
> Can you remove the compile time assert?

Done.

> We usually check stuff like
> this using configure.  If you can send in a patch for configure.ac,
> that would be appreciated.
>
>   
Yes, but be patient.

The configure approach is quite different: In configure, you can check 
whether compiler supports attr packed and whether it works as expected. 
With the good old typedef compile time assert, you can check whether 
this *specific* structure is properly declared and packed.


> > --- grub2.orig/include/grub/i386/pc/init.h      2007-07-22 01:32:23.000000000 
> > +0200
> > +++ grub2/include/grub/i386/pc/init.h   2007-10-13 21:25:24.000000000 +0200
> > @@ -40,10 +40,14 @@ grub_uint32_t grub_get_eisa_mmap (void);
> >  struct grub_machine_mmap_entry
> >  {
> >    grub_uint32_t size;
> > -  grub_uint64_t addr;
> > +  grub_uint64_t addr; /* must be at offset 4, see startup.S */ 
> >     
>
> I do not think this comment is required.  It's fixed now :-)
>
>   

Hmm...OK removed. Now there is no clue why this struct has packing 
requirements. And this also is no longer checked.

Christian


2007-11-09  Christian Franke  <address@hidden>

        * include/grub/i386/pc/init.h (struct grub_machine_mmap_entry):
        Add attribute packed, gcc 3.4.4 on Cygwin aligns this
        to 64 bit boundary by default.

--- grub2.orig/include/grub/i386/pc/init.h      2007-10-31 23:55:57.093750000 
+0100
+++ grub2/include/grub/i386/pc/init.h   2007-11-09 20:27:52.312500000 +0100
@@ -39,7 +39,7 @@
   grub_uint64_t addr;
   grub_uint64_t len;
   grub_uint32_t type;
-};
+} __attribute__((packed));
 
 /* Get a memory map entry. Return next continuation value. Zero means
    the end.  */