[U-Boot] [PATCH] fat: fix unaligned errors
From: Bryan Wu bryan.wu@analog.com
A couple of buffers in the fat code are declared as an array of bytes. But it is then cast up to a structure with 16bit and 32bit members. Since GCC assumes structure alignment here, we have to force the buffers to be aligned according to the structure usage.
Signed-off-by: Bryan Wu bryan.wu@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org --- fs/fat/fat.c | 19 +++++++++++++++---- 1 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/fs/fat/fat.c b/fs/fat/fat.c index a9dde7d..06736d9 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -429,12 +429,16 @@ slot2str(dir_slot *slotptr, char *l_name, int *idx)
/* + * We need align this buffer to 16bit, cause it will be cast up to + * a dir_slot structure + */ +__u8 get_vfatname_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u16)))); +/* * Extract the full long filename starting at 'retdent' (which is really * a slot) into 'l_name'. If successful also copy the real directory entry * into 'retdent' * Return 0 on success, -1 otherwise. */ -__u8 get_vfatname_block[MAX_CLUSTSIZE]; static int get_vfatname(fsdata *mydata, int curclust, __u8 *cluster, dir_entry *retdent, char *l_name) @@ -517,10 +521,14 @@ mkcksum(const char *str)
/* + * We need align this buffer to 32bit, cause it will be cast up to + * a dir_entry structure + */ +__u8 get_dentfromdir_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32)))); +/* * Get the directory entry associated with 'filename' from the directory * starting at 'startsect' */ -__u8 get_dentfromdir_block[MAX_CLUSTSIZE]; static dir_entry *get_dentfromdir (fsdata * mydata, int startsect, char *filename, dir_entry * retdent, int dols) @@ -725,8 +733,11 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize) return -1; }
- -__u8 do_fat_read_block[MAX_CLUSTSIZE]; /* Block buffer */ +/* + * We need align this buffer to 32bit, cause it will be cast up to + * a dir_entry structure + */ +__u8 do_fat_read_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32)))); long do_fat_read (const char *filename, void *buffer, unsigned long maxsize, int dols)
Hello Mike,
2009/1/2 Mike Frysinger vapier@gentoo.org:
From: Bryan Wu bryan.wu@analog.com A couple of buffers in the fat code are declared as an array of bytes. But it is then cast up to a structure with 16bit and 32bit members. Since GCC assumes structure alignment here, we have to force the buffers to be aligned according to the structure usage.
Just curious (because before the Christmas holidays I was also debugging some FAT problems, and maybe this is related) What problem/symptoms does this patch fix? How did the bug show itself?
Kind Regards,
Remy
Signed-off-by: Bryan Wu bryan.wu@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org
fs/fat/fat.c | 19 +++++++++++++++---- 1 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/fs/fat/fat.c b/fs/fat/fat.c index a9dde7d..06736d9 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -429,12 +429,16 @@ slot2str(dir_slot *slotptr, char *l_name, int *idx)
/*
- We need align this buffer to 16bit, cause it will be cast up to
- a dir_slot structure
- */
+__u8 get_vfatname_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u16)))); +/*
- Extract the full long filename starting at 'retdent' (which is really
- a slot) into 'l_name'. If successful also copy the real directory entry
- into 'retdent'
- Return 0 on success, -1 otherwise.
*/ -__u8 get_vfatname_block[MAX_CLUSTSIZE]; static int get_vfatname(fsdata *mydata, int curclust, __u8 *cluster, dir_entry *retdent, char *l_name) @@ -517,10 +521,14 @@ mkcksum(const char *str)
/*
- We need align this buffer to 32bit, cause it will be cast up to
- a dir_entry structure
- */
+__u8 get_dentfromdir_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32)))); +/*
- Get the directory entry associated with 'filename' from the directory
- starting at 'startsect'
*/ -__u8 get_dentfromdir_block[MAX_CLUSTSIZE]; static dir_entry *get_dentfromdir (fsdata * mydata, int startsect, char *filename, dir_entry * retdent, int dols) @@ -725,8 +733,11 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize) return -1; }
-__u8 do_fat_read_block[MAX_CLUSTSIZE]; /* Block buffer */ +/*
- We need align this buffer to 32bit, cause it will be cast up to
- a dir_entry structure
- */
+__u8 do_fat_read_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32)))); long do_fat_read (const char *filename, void *buffer, unsigned long maxsize, int dols) -- 1.6.0.6
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
On Friday 02 January 2009 04:32:39 Remy Bohmer wrote:
2009/1/2 Mike Frysinger vapier@gentoo.org:
From: Bryan Wu bryan.wu@analog.com A couple of buffers in the fat code are declared as an array of bytes. But it is then cast up to a structure with 16bit and 32bit members. Since GCC assumes structure alignment here, we have to force the buffers to be aligned according to the structure usage.
Just curious (because before the Christmas holidays I was also debugging some FAT problems, and maybe this is related) What problem/symptoms does this patch fix? How did the bug show itself?
i think should cover all your questions ... http://blackfin.uclinux.org/gf/tracker/4765 -mike
Dear Mike Frysinger,
In message 1230857672-11798-1-git-send-email-vapier@gentoo.org you wrote:
From: Bryan Wu bryan.wu@analog.com
A couple of buffers in the fat code are declared as an array of bytes. But it is then cast up to a structure with 16bit and 32bit members. Since GCC assumes structure alignment here, we have to force the buffers to be aligned according to the structure usage.
...
+__u8 get_vfatname_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u16))));
...
+__u8 get_dentfromdir_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));
...
+__u8 do_fat_read_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));
What makes you sure that a 16 resp. 32 bit alignment is sufficient, and that gcc does not decide to align such structures even stricter?
Wouldn't it make more sense to use "__alignof__ ()" here to be on the safe side?
Best regards,
Wolfgang Denk
On Friday 02 January 2009 17:20:47 Wolfgang Denk wrote:
In message vapier@gentoo.org you wrote:
From: Bryan Wu bryan.wu@analog.com
A couple of buffers in the fat code are declared as an array of bytes. But it is then cast up to a structure with 16bit and 32bit members. Since GCC assumes structure alignment here, we have to force the buffers to be aligned according to the structure usage.
...
+__u8 get_vfatname_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u16))));
...
+__u8 get_dentfromdir_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));
...
+__u8 do_fat_read_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));
What makes you sure that a 16 resp. 32 bit alignment is sufficient, and that gcc does not decide to align such structures even stricter?
read the structures in question. the largest member is 16bit/32bit. i really dont know what other alignment gcc could randomly generate.
Wouldn't it make more sense to use "__alignof__ ()" here to be on the safe side?
i found no such alignment attribute in the gcc docs which is why i suggested Bryan use aligned(sizeof(...)). http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Function-Attributes.html http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Variable-Attributes.html http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Type-Attributes.html -mike
Dear Mike,
In message 200901021742.05363.vapier@gentoo.org you wrote:
+__u8 get_vfatname_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u16))));
...
+__u8 get_dentfromdir_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));
...
+__u8 do_fat_read_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));
What makes you sure that a 16 resp. 32 bit alignment is sufficient, and that gcc does not decide to align such structures even stricter?
read the structures in question. the largest member is 16bit/32bit. i really dont know what other alignment gcc could randomly generate.
It is not (only) the size of the "largest" member in a struct which determins global alignment, but also the size of the struct itself.
See for example http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Type-Attributes.html
Read especially this part:
In the example above, if the size of each short is 2 bytes, then the size of the entire struct S type is 6 bytes. The smallest power of two which is greater than or equal to that is 8, so the compiler sets the alignment for the entire struct S type to 8 bytes.
Wouldn't it make more sense to use "__alignof__ ()" here to be on the safe side?
i found no such alignment attribute in the gcc docs which is why i suggested Bryan use aligned(sizeof(...)). http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Function-Attributes.html http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Variable-Attributes.html http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Type-Attributes.html
Maybe you should have searched for the (IMHO pretty obvious) term "Alignment" as well, i.e. look at http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Alignment.html#Alignment
Quote:
The keyword __alignof__ allows you to inquire about how an object is aligned, or the minimum alignment usually required by a type. Its syntax is just like sizeof.
Best regards,
Wolfgang Denk
On Friday 02 January 2009 18:48:50 Wolfgang Denk wrote:
In message 200901021742.05363.vapier@gentoo.org you wrote:
+__u8 get_vfatname_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u16))));
...
+__u8 get_dentfromdir_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));
...
+__u8 do_fat_read_block[MAX_CLUSTSIZE] __attribute__ ((aligned(sizeof(__u32))));
What makes you sure that a 16 resp. 32 bit alignment is sufficient, and that gcc does not decide to align such structures even stricter?
read the structures in question. the largest member is 16bit/32bit. i really dont know what other alignment gcc could randomly generate.
It is not (only) the size of the "largest" member in a struct which determins global alignment, but also the size of the struct itself.
See for example http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Type-Attributes.html
Read especially this part:
In the example above, if the size of each short is 2 bytes, then the size of the entire struct S type is 6 bytes. The smallest power of two which is greater than or equal to that is 8, so the compiler sets the alignment for the entire struct S type to 8 bytes.
i was not aware of this, thanks for the highlight
Wouldn't it make more sense to use "__alignof__ ()" here to be on the safe side?
i found no such alignment attribute in the gcc docs which is why i suggested Bryan use aligned(sizeof(...)). http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Function-Attributes.html http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Variable-Attributes.html http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Type-Attributes.html
Maybe you should have searched for the (IMHO pretty obvious) term "Alignment" as well, i.e. look at http://gcc.gnu.org/onlinedocs/gcc-4.3.2/gcc/Alignment.html#Alignment
Quote:
The keyword __alignof__ allows you to inquire about how an object is aligned, or the minimum alignment usually required by a type. Its syntax is just like sizeof.
well i guess we cant all be "Wolfgang Denk"s -mike
From: Bryan Wu bryan.wu@analog.com
A couple of buffers in the fat code are declared as an array of bytes. But it is then cast up to a structure with 16bit and 32bit members. Since GCC assumes structure alignment here, we have to force the buffers to be aligned according to the structure usage.
Signed-off-by: Bryan Wu bryan.wu@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org --- v1 - use alignof() rather than sizeof()
fs/fat/fat.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/fat/fat.c b/fs/fat/fat.c index a9dde7d..28c7805 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -434,7 +434,8 @@ slot2str(dir_slot *slotptr, char *l_name, int *idx) * into 'retdent' * Return 0 on success, -1 otherwise. */ -__u8 get_vfatname_block[MAX_CLUSTSIZE]; +__attribute__ ((__aligned__(__alignof__(dir_entry)))) +__u8 get_vfatname_block[MAX_CLUSTSIZE]; static int get_vfatname(fsdata *mydata, int curclust, __u8 *cluster, dir_entry *retdent, char *l_name) @@ -520,6 +521,7 @@ mkcksum(const char *str) * Get the directory entry associated with 'filename' from the directory * starting at 'startsect' */ +__attribute__ ((__aligned__(__alignof__(dir_entry)))) __u8 get_dentfromdir_block[MAX_CLUSTSIZE]; static dir_entry *get_dentfromdir (fsdata * mydata, int startsect, char *filename, dir_entry * retdent, @@ -725,8 +727,8 @@ read_bootsectandvi(boot_sector *bs, volume_info *volinfo, int *fatsize) return -1; }
- -__u8 do_fat_read_block[MAX_CLUSTSIZE]; /* Block buffer */ +__attribute__ ((__aligned__(__alignof__(dir_entry)))) +__u8 do_fat_read_block[MAX_CLUSTSIZE]; long do_fat_read (const char *filename, void *buffer, unsigned long maxsize, int dols)
Dear Mike Frysinger,
In message 1230947265-19412-1-git-send-email-vapier@gentoo.org you wrote:
From: Bryan Wu bryan.wu@analog.com
A couple of buffers in the fat code are declared as an array of bytes. But it is then cast up to a structure with 16bit and 32bit members. Since GCC assumes structure alignment here, we have to force the buffers to be aligned according to the structure usage.
Signed-off-by: Bryan Wu bryan.wu@analog.com Signed-off-by: Mike Frysinger vapier@gentoo.org
v1
- use alignof() rather than sizeof()
fs/fat/fat.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
participants (3)
-
Mike Frysinger -
Remy Bohmer -
Wolfgang Denk