gcc/binutils bug building parted?
Hi,
Running:
ii binutils 2.11.93.0.2-2 The GNU assembler, linker and binary utilities. ii gcc 2.96-13 The GNU C compiler.
and building:
ii parted 1.4.24-1 The GNU Parted disk partition resizing program
I use parted to create GPT tables, which have a partition type GUID in the first 16 bytes of each table entry. That entry is being written offset by one byte, so a partition table entry might look like
0000480 00 a2 a0 d0 eb e5 b9 33 44 87 c0 68 b6 b7 26 99 0000490 83 79 f1 c4 f9 32 46 f7 9a 51 cc 85 c5 d8 a9 68 00004a0 00 28 03 00 00 00 00 00 00 40 06 00 00 00 00 00 00004b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
instead of
0000480 a2 a0 d0 eb e5 b9 33 44 87 c0 68 b6 b7 26 99 c7 0000490 83 79 f1 c4 f9 32 46 f7 9a 51 cc 85 c5 d8 a9 68 00004a0 00 28 03 00 00 00 00 00 00 40 06 00 00 00 00 00 00004b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
I've included the relevant bit of code below, along with a disassembly and a dump of the relevant bit of .rodata. The significant bit is that "gpt_part_data->pte->PartitionTypeGuid = PARTITION_BASIC_DATA_GUID" is implemented as a memcpy(), with a source of .rodata+0xe7, while we can see in the dump that the guid is actually at .rodata+0xe8.
I've never looked at ia64 asm before today, but I think I've interpreted it correctly. This is under debian, but the same corruption has been seen under RedHat 7.2.
Richard
include/parted/disk_gpt.h:
typedef struct {
uint32_t time_low;
uint16_t time_mid;
uint16_t time_hi_and_version;
uint8_t clock_seq_hi_and_reserved;
uint8_t clock_seq_low;
uint8_t node[6];
} __attribute__ ((packed)) efi_guid_t;#define PARTITION_BASIC_DATA_GUID \
((efi_guid_t) { 0xEBD0A0A2, 0xB9E5, 0x4433, 0x87, 0xC0, { 0x68, 0xB6, 0xB7, 0x26, 0x99, 0xC7 }})libparted/fs_ext2/interface.c
if (strcmp (disk_type->name, GPT_NAME) = 0) {
GPTPartitionData *gpt_part_data = part->disk_specific;
PED_ASSERT(gpt_part_data != NULL, return 0);
PED_ASSERT(gpt_part_data->pte != NULL, return 0);
gpt_part_data->pte->PartitionTypeGuid PARTITION_BASIC_DATA_GUID;
{
uint8_t *p = (uint8_t *)&gpt_part_data->pte->PartitionTypeGuid;
printf("%p: %02x %02x\n", p, p[0], p[1]);
}
return 1;
}objdump -dr build/libparted/fs_ext2/interface.o:
1330: 1d 58 01 56 18 10 [MFB] ld8 r43=[r43]
1332: PCREL21B ped_assert
1336: 00 00 00 02 00 00 nop.f 0x0
133c: 08 00 00 50 br.call.sptk.many b030 <_ext2_set_system+0x700>;;
1340: 02 70 00 10 00 21 [MII] mov r14=r8
1346: 10 00 8c 00 42 e0 mov r1=r35;;
134c: 00 70 18 e6 cmp4.eq p7,p6=0,r14
1350: 1c 00 00 00 01 00 [MFB] nop.m 0x0
1356: 00 00 00 02 80 03 nop.f 0x0
135c: b0 01 00 43 (p07) br.cond.dpnt.few 1500 <_ext2_set_system+0x8d0>
1360: 09 40 01 02 00 24 [MMI] addl r40=0,r1
1360: LTOFF22 .rodata+0xe7
1366: 70 02 84 30 20 20 ld8 r39=[r33] # r39=dst
136c: 05 01 00 90 mov r41;; # r41=cnt
1370: 1d 40 01 50 18 10 [MFB] ld8 r40=[r40] # r40=src, .rodata+0xe7
1372: PCREL21B memcpy
1376: 00 00 00 02 00 00 nop.f 0x0
137c: 08 00 00 50 br.call.sptk.many b070 <_ext2_set_system+0x740>;;
1380: 02 40 01 42 18 10 [MII] ld8 r40=[r33] # p = gpt_part_data->pte->PartitionTypeGuid
1386: 10 00 8c 00 42 c0 mov r1=r35;;
138c: 01 40 01 84 mov r14=r40
1390: 0a 38 01 02 00 24 [MMI] addl r39=0,r1;; # printf fmt string
1390: LTOFF22 .rodata.str1.8+0x2b0
1396: 90 0a 38 00 28 00 ld1 r41=[r14],1 # p[1]
139c: 00 00 04 00 nop.i 0x0
13a0: 0a 38 01 4e 18 10 [MMI] ld8 r39=[r39];;
13a6: a0 02 38 00 20 00 ld1 r42=[r14] # p[0]
13ac: 00 00 04 00 nop.i 0x0
13b0: 1d 00 00 00 01 00 [MFB] nop.m 0x0
13b2: PCREL21B printf
13b6: 00 00 00 02 00 00 nop.f 0x0
13bc: 08 00 00 50 br.call.sptk.many b0b0 <_ext2_set_system+0x780>;;objdump -d -j .rodata build/libparted/fs_ext2/interface.o:
...
...00000000000000b0 <_ext2_fs_h>:
b0: 24 49 64 3a 20 65 78 74 32 5f 66 73 2e 68 2c 76 $Id: ext2_fs.h,v c0: 20 31 2e 33 20 31 39 39 39 2f 31 32 2f 33 31 20 1.3 1999/12/31 d0: 30 39 3a 33 36 3a 34 31 20 62 75 79 74 65 6e 68 09:36:41 buytenh e0: 20 45 78 70 20 24 00 00 a2 a0 d0 eb e5 b9 33 44 Exp $........3D f0: 87 c0 68 b6 b7 26 99 c7 ..h..&..
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Richard Hirst
Richard> I use parted to create GPT tables, which have a partition Richard> type GUID in the first 16 bytes of each table entry. That Richard> entry is being written offset by one byte, so a partition Richard> table entry might look like
Richard> 0000480 00 a2 a0 d0 eb e5 b9 33 44 87 c0 68 b6 b7 26 99 Richard> 0000490 83 79 f1 c4 f9 32 46 f7 9a 51 cc 85 c5 d8 a9 68 Richard> 00004a0 00 28 03 00 00 00 00 00 00 40 06 00 00 00 00 00 Richard> 00004b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Richard> instead of
Richard> 0000480 a2 a0 d0 eb e5 b9 33 44 87 c0 68 b6 b7 26 99 c7 Richard> 0000490 83 79 f1 c4 f9 32 46 f7 9a 51 cc 85 c5 d8 a9 68 Richard> 00004a0 00 28 03 00 00 00 00 00 00 40 06 00 00 00 00 00 Richard> 00004b0 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Richard> I've included the relevant bit of code below, along with a Richard> disassembly and a dump of the relevant bit of .rodata. The Richard> significant bit is that
Richard> "gpt_part_data->pte->PartitionTypeGuid Richard> PARTITION_BASIC_DATA_GUID" is implemented as a memcpy(),
Richard> with a source of .rodata+0xe7, while we can see in the dump Richard> that the guid is actually at .rodata+0xe8.
Richard> I've never looked at ia64 asm before today, but I think Richard> I've interpreted it correctly. This is under debian, but Richard> the same corruption has been seen under RedHat 7.2.
That is pretty weird. Does the problem occur with gcc3 as well? It might also help to look at the assembly code.
--david
Quoted 2 lines · David Mosberger
That is pretty weird. Does the problem occur with gcc3 as well? It might also help to look at the assembly code.
Yes, it does happen with gcc 3.0.4 as well, if I mess with a string length in the .c source, to provoke it. The problem seems to be related to the packed attribute on the struct definition:
typedef struct {
uint32_t time_low;
uint16_t time_mid;
uint16_t time_hi_and_version;
uint8_t clock_seq_hi_and_reserved;
uint8_t clock_seq_low;
uint8_t node[6];
} __attribute__ ((packed)) efi_guid_t;When that is used thus:
#define PARTITION_BASIC_DATA_GUID \
((efi_guid_t) { 0xEBD0A0A2, 0xB9E5, 0x4433, 0x87, 0xC0, { 0x68, 0xB6, 0xB7, 0x26, 0x99, 0xC7 }})
gpt_part_data->pte->PartitionTypeGuid PARTITION_BASIC_DATA_GUID;it generates assembler like this:
.section .rodata
.LC26:
data4 3956318370
data2 47589
data2 17459
data1 135
data1 192
data1 104
data1 182
data1 183
data1 38
data1 153
data1 199
...
...
addl r40 = @ltoff(.LC26), gp...
Note there is no .align before that .LC26 label. The last byte used in .rodata happens to be 0xe6, so r40 gets loaded with .rodata+0xe7, as can be seen if I disassemble the .o file:
1360: 09 40 01 02 00 24 [MMI] addl r40=0,r1
1360: LTOFF22 .rodata+0xe7That is all correct, I guess, but when I come to dump the .rodata section I see
d0: 30 39 3a 33 36 3a 34 31 20 62 75 79 74 65 6e 68 09:36:41 buytenh e0: 20 45 78 70 20 24 00 00 a2 a0 d0 eb e5 b9 33 44 Exp $........3D f0: 87 c0 68 b6 b7 26 99 c7 ..h..&..
where you can see that the LC26 data is actually stored at .rodata+0xe8, not +0xe7.
If I remove the __attribute__ ((packed)), then the assembler has a .align 4 before .LC26, and things are ok. It seems odd that strings in .rodata are all .align 8, while this struct, even when not packed, is only .align 4.
It looks like the code generation assumes no .align to mean .align 1, while the .rodata generation assumes no .align to mean .align 4. Is that possible?
Given that that struct is arranged with largest types first (as dictated by the guid definition, of course), is there any reason to have a packed attribute on it anyway?
The attached program will generate the problem for you; "ding" is 5 bytes long so the generated code thinks the guid is at .rodata+5, while .rodata is actually generated with the guid at .rodata+8.
Richard
---------------------------------- cut ------------------------------- typedef unsigned int uint32_t;
typedef unsigned short uint16_t;
typedef unsigned char uint8_t;
typedef struct {
uint32_t time_low;
uint16_t time_mid;
uint16_t time_hi_and_version;
uint8_t clock_seq_hi_and_reserved;
uint8_t clock_seq_low;
uint8_t node[6];
} __attribute__ ((packed)) efi_guid_t;#define PARTITION_BASIC_DATA_GUID \
((efi_guid_t) { 0xEBD0A0A2, 0xB9E5, 0x4433, 0x87, 0xC0, { 0x68, 0xB6, 0xB7, 0x26, 0x99, 0xC7 }})void
bar (char **p)
{
*p = "ding";
}void
foo (efi_guid_t *g)
{
*g = PARTITION_BASIC_DATA_GUID;
}int
main (int argc, char **argv)
{
efi_guid_t g;
char *p;
bar(&p);
foo(&g);
return 0;
}---------------------------------- cut -------------------------------
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Richard Hirst
Richard> Note there is no .align before that .LC26 label. The last Richard> byte used in .rodata happens to be 0xe6, so r40 gets loaded Richard> with .rodata+0xe7, as can be seen if I disassemble the .o Richard> file:
Richard> 1360: 09 40 01 02 00 24 [MMI] addl r40=0,r1 1360:
Richard> LTOFF22 .rodata+0xe7
Richard> That is all correct, I guess, but when I come to dump the Richard> .rodata section I see
Richard> d0: 30 39 3a 33 36 3a 34 31 20 62 75 79 74 65 6e 68
Richard> 09:36:41 buytenh e0: 20 45 78 70 20 24 00 00 a2 a0 d0 eb e5 Richard> b9 33 44 Exp $........3D f0: 87 c0 68 b6 b7 26 99 c7 Richard> ..h..&..
Richard> where you can see that the LC26 data is actually stored at Richard> .rodata+0xe8, not +0xe7.
Oh, I see now what's wrong: data4 implicitly aligns the data to a 4-byte boundary. To get an unaligned entry, you'd have to use data4.ua or gcc would have to emit the data byte by byte (data1).
This needs to be fixed in the compiler. Would you mind filing a bug report with the gcc folks?
However, I also feel strongly the defining the efi_guid_t like this:
typedef struct {
uint32_t time_low;
uint16_t time_mid;
uint16_t time_hi_and_version;
uint8_t clock_seq_hi_and_reserved;
uint8_t clock_seq_low;
uint8_t node[6];
} __attribute__ ((packed)) efi_guid_t;is fundamentally broken, because it introduces byte-order dependency. I'd recommend doing something along the following lines instead:
typedef unsigned char guid_t[16];
#define EFI_GUID(a,b,c,b0,b1,b2,b3,b4,b5,b6,b7) \
{ (a) & 0xff, ((a) >> 8) & 0xff, ((a) >> 16) & 0xff, ((a) >> 24) & 0xff, \
(b) & 0xff, ((b) >> 8) & 0xff, \
(c) & 0xff, ((c) >> 8) & 0xff, \
(b0), (b1), (b2), (b3), (b4), (b5), (b6), (b7) }
This is untested, but I think you'll get the idea. This should be much better because it works independent of the host's byte-order and doesn't require using gcc-only extensions to C. It will also work around the compiler bug until that gets fixed.
Now if only we could convince the EFI folks to do the same...
--david
Quoted 2 lines
typedef unsigned char guid_t[16];
#defineEFI_GUID(a,b,c,b0,b1,b2,b3,b4,b5,b6,b7)
I meant to do this in the kernel's fs/partition/efi.[ch] files too, but never got around to it... Back on the TODO list.
--
Matt Domsch
Sr. Software Engineer
Dell Linux Solutions www.dell.com/linux
Linux on Dell mailing lists @ lists.us.dell.com
#1 US Linux Server provider for 2001!
Quoted 6 lines · David Mosberger
Oh, I see now what's wrong: data4 implicitly aligns the data to a 4-byte boundary. To get an unaligned entry, you'd have to use data4.ua or gcc would have to emit the data byte by byte (data1).
This needs to be fixed in the compiler. Would you mind filing a bug report with the gcc folks?
Will do.
Quoted 2 lines
typedef unsigned char guid_t[16];
#defineEFI_GUID(a,b,c,b0,b1,b2,b3,b4,b5,b6,b7)
OK, thanks. I'll work up a patch for parted to do that if no-one beats me to it.
Cheers,
Richard
Quoted 5 lines · Richard Hirst
Quoted 2 lines · David Mosberger
This needs to be fixed in the compiler. Would you mind filing a bug report with the gcc folks?
Will do.
For reference, the gcc-gnats bug refs are
c/5973: gcc 2.96 generates bad code to reference ((packed)) stru c/5974: gcc 3.0.4 generates bad code to reference ((packed)) str
Richard