Re: [PATCH] Fix packing issue of machine_mmap_entry

[Marco Gerards]

Christian Franke <address@hidden> writes:

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

Right, but if you add a configure.ac option you can see if it works
globally.  If it doesn't, we have a problem anyways.  We shouldn't
rely on how it is packed.  If the packed attribute doesn't work, GRUB
can't be compiled.

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

All structs that rely on gcc not adding packing for alignment, should
have the packed attribute.

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

This looks good :-)

--
Marco