[PATCH] Fix packing issue of machine_mmap_entry

[PATCH] Fix packing issue of machine_mmap_entry

[Christian Franke]

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

First patch related to the Cygwin port:

struct machine_mmap_entry is sensitive to packing of 64 bit values which 
apparently differs between gcc releases.

Christian

2007-10-22  Christian Franke  <franke@computer.org>

	* 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.
	Add compile time assert to check packing.




[-- Attachment #2: grub2-machine_mmap_entry.patch --]
[-- Type: text/x-patch, Size: 691 bytes --]

--- 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 */ 
   grub_uint64_t len;
   grub_uint32_t type;
-};
+} __attribute__((packed));
+
+/* Compile time assert to check packing */
+typedef char ASSERT_sizeof_grub_machine_mmap_entry[
+  sizeof (struct grub_machine_mmap_entry) == 4+8+8+4 ? 1 : -1];
 
 /* Get a memory map entry. Return next continuation value. Zero means
    the end.  */

Re: [PATCH] Fix packing issue of machine_mmap_entry

[Robert Millan]

On Mon, Oct 22, 2007 at 11:00:11PM +0200, Christian Franke wrote:
> --- 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 */ 
>    grub_uint64_t len;
>    grub_uint32_t type;
> -};
> +} __attribute__((packed));

Looks fine to me (I suppose we would have the same problem on x86_64).

> +/* Compile time assert to check packing */
> +typedef char ASSERT_sizeof_grub_machine_mmap_entry[
> +  sizeof (struct grub_machine_mmap_entry) == 4+8+8+4 ? 1 : -1];

Nice, I didn't know you could do compile time assertion in C.  But is it
really necessary to check for gcc bugs?  That __attribute__((packed)) can
only have the expected effect, right?  Besides, in this case maybe it'd
be better to use autoconf.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

Re: [PATCH] Fix packing issue of machine_mmap_entry

[Christian Franke]

Robert Millan wrote:
> ...
> > +/* Compile time assert to check packing */
> > +typedef char ASSERT_sizeof_grub_machine_mmap_entry[
> > +  sizeof (struct grub_machine_mmap_entry) == 4+8+8+4 ? 1 : -1];
> 
> Nice, I didn't know you could do compile time assertion in C.  But is
> it really necessary to check for gcc bugs?  That
> __attribute__((packed)) can only have the expected effect, right? 
> Besides, in this case maybe it'd be better to use autoconf.
> 

The check is not really necessary here, as the struct is simple, the
module is i386-pc only, and the project is gcc-only (e.g. due to nested
functions). I've added it during debugging of the Cygwin release, feel
free to remove it.

The compile time assert trick comes in handy for larger structs with
packing requirements, e.g.:
http://smartmontools.cvs.sourceforge.net/smartmontools/sm5/atacmds.h?r1=1.75&r2=1.76
(In this project, we got at least one gcc variant not supporting
__attribute__((packed)))

Another example: The code requires some size of a simple type:
  typedef char ASSERT_off_t_is_64bit[sizeof(off_t) == 8 ? 1 : -1];

Christian

Re: [PATCH] Fix packing issue of machine_mmap_entry

[Marco Gerards]

Christian Franke <Christian.Franke@t-online.de> writes:

> First patch related to the Cygwin port:
>
> struct machine_mmap_entry is sensitive to packing of 64 bit values
> which apparently differs between gcc releases.

Thanks!

> Christian
>
> 2007-10-22  Christian Franke  <franke@computer.org>
>
> 	* 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.
> 	Add compile time assert to check packing.

Can you remove the compile time assert?  We usually check stuff like
this using configure.  If you can send in a patch for configure.ac,
that would be appreciated.

>
>
> --- 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 :-)

>    grub_uint64_t len;
>    grub_uint32_t type;
> -};
> +} __attribute__((packed));

--
Marco

Re: [PATCH] Fix packing issue of machine_mmap_entry

[Christian Franke]

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

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  <franke@computer.org>

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


[-- Attachment #2: grub2-machine_mmap_entry-2.patch --]
[-- Type: text/x-patch, Size: 361 bytes --]

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

Re: [PATCH] Fix packing issue of machine_mmap_entry

[Marco Gerards]

Christian Franke <Christian.Franke@t-online.de> 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  <franke@computer.org>
>
> 	* 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

Re: [PATCH] Fix packing issue of machine_mmap_entry

[Robert Millan]

On Sat, Nov 10, 2007 at 04:55:45PM +0100, Marco Gerards wrote:
> >
> > 2007-11-09  Christian Franke  <franke@computer.org>
> >
> > 	* 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 :-)

Committed.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)