Fix packing issue of machine_mmap_entry

PATCH In review Christian Franke 3 participants 7 messages Oct 22, 2007 – Nov 18, 2007

lore

Share & tools

lore

Christian Franke

author

2007-10-22 21:01 UTC #0 lore raw

PATCH 1/1 Fix packing issue of machine_mmap_entry

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 <[email protected]>

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

include/grub/i386/pc/init.h +6 -2

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

Robert Millan

2007-10-23 06:02 UTC #1 lore raw

include/grub/i386/pc/init.h grub_uint32_t grub_get_eisa_mmap (void); · line 46

Quoted 12 lines · Christian Franke

include/grub/i386/pc/init.h +2 -2

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

include/grub/i386/pc/init.h grub_uint32_t grub_get_eisa_mmap (void); · line 50

Quoted 3 lines

include/grub/i386/pc/init.h +3 -0

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

Signature · 5 lines

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

Christian Franke

author

2007-10-23 08:09 UTC #2 lore raw

Quoted 10 lines · Robert Millan

...
include/grub/i386/pc/init.h grub_uint32_t grub_get_eisa_mmap (void); · line 50
Quoted 3 lines
include/grub/i386/pc/init.h +3 -0
+/* 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

Marco Gerards

2007-11-09 14:00 UTC #3 lore raw

In reply to Christian Franke

Quoted 4 lines · Christian Franke

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!

Quoted 8 lines

Christian
2007-10-22 Christian Franke <[email protected]>
* 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.

include/grub/i386/pc/init.h grub_uint32_t grub_get_eisa_mmap (void); · line 43

Quoted 10 lines

include/grub/i386/pc/init.h +1 -1

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

include/grub/i386/pc/init.h grub_uint32_t grub_get_eisa_mmap (void); · line 46

Quoted 4 lines

include/grub/i386/pc/init.h +1 -1

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

--
Marco

Christian Franke

author

2007-11-09 19:56 UTC #4 lore raw

Quoted 5 lines · Marco Gerards

Quoted 3 lines
...
Add compile time assert to check packing.
Can you remove the compile time assert?

Done.

Quoted 5 lines

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.

Quoted 13 lines

include/grub/i386/pc/init.h grub_uint32_t grub_get_eisa_mmap (void); · line 44
Quoted 9 lines
include/grub/i386/pc/init.h +1 -1
@@ -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 <[email protected]>

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

include/grub/i386/pc/init.h +1 -1

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

Marco Gerards

2007-11-10 15:55 UTC #5 lore raw

Quoted 15 lines · Christian Franke

Quoted 5 lines · Marco Gerards
Quoted 3 lines
...
Add compile time assert to check packing.
Can you remove the compile time assert?
Done.
Quoted 5 lines
We usually check stuff like
this using configure. If you can send in a patch for configure.ac, that would be appreciated.

Quoted 6 lines

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.

Quoted 13 lines

Quoted 12 lines
include/grub/i386/pc/init.h grub_uint32_t grub_get_eisa_mmap (void); · line 43
Quoted 8 lines
include/grub/i386/pc/init.h +1 -1
@@ -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 :-)

Quoted 2 lines

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.

Quoted 8 lines

Christian
2007-11-09 Christian Franke <[email protected]>
* 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

Robert Millan

2007-11-18 07:11 UTC #6 lore raw

Quoted 8 lines · Marco Gerards

Quoted 6 lines
2007-11-09 Christian Franke <[email protected]>
* 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.

Signature · 5 lines

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