[U-Boot] [PATCH] ARM: prevent misaligned array inits
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net --- arch/arm/cpu/arm926ejs/orion5x/cpu.c | 4 +- arch/arm/lib/interrupts.c | 2 +- board/ti/omap2420h4/sys_info.c | 28 ++++----- common/Makefile | 3 + common/cmd_dfu.c | 2 +- doc/README.arm-unaligned-accesses | 110 ++++++++++++++++++++++++++++++++++ fs/fat/Makefile | 2 + fs/ubifs/Makefile | 3 + lib/Makefile | 3 + 9 files changed, 139 insertions(+), 18 deletions(-) create mode 100644 doc/README.arm-unaligned-accesses
diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c b/arch/arm/cpu/arm926ejs/orion5x/cpu.c index c3948d3..5a4775a 100644 --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c @@ -194,8 +194,8 @@ u32 orion5x_device_rev(void) */ int print_cpuinfo(void) { - char dev_str[] = "0x0000"; - char rev_str[] = "0x00"; + char dev_str[7]; /* room enough for 0x0000 plus null byte */ + char rev_str[5]; /* room enough for 0x00 plus null byte */ char *dev_name = NULL; char *rev_name = NULL;
diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c index 74ff5ce..2f97933 100644 --- a/arch/arm/lib/interrupts.c +++ b/arch/arm/lib/interrupts.c @@ -169,7 +169,7 @@ void do_prefetch_abort (struct pt_regs *pt_regs)
void do_data_abort (struct pt_regs *pt_regs) { - printf ("data abort\n"); + printf ("data abort\n\n MAYBE you should read doc/README.unaligned-access\n\n"); show_regs (pt_regs); bad_mode (); } diff --git a/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c index a9f7241..b12011e 100644 --- a/board/ti/omap2420h4/sys_info.c +++ b/board/ti/omap2420h4/sys_info.c @@ -237,20 +237,20 @@ u32 wait_on_value(u32 read_bit_mask, u32 match_value, u32 read_addr, u32 bound) *********************************************************************/ void display_board_info(u32 btype) { - char cpu_2420[] = "2420"; /* cpu type */ - char cpu_2422[] = "2422"; - char cpu_2423[] = "2423"; - char db_men[] = "Menelaus"; /* board type */ - char db_ip[] = "IP"; - char mem_sdr[] = "mSDR"; /* memory type */ - char mem_ddr[] = "mDDR"; - char t_tst[] = "TST"; /* security level */ - char t_emu[] = "EMU"; - char t_hs[] = "HS"; - char t_gp[] = "GP"; - char unk[] = "?"; - - char *cpu_s, *db_s, *mem_s, *sec_s; + static const char cpu_2420 [] = "2420"; /* cpu type */ + static const char cpu_2422 [] = "2422"; + static const char cpu_2423 [] = "2423"; + static const char db_men [] = "Menelaus"; /* board type */ + static const char db_ip [] = "IP"; + static const char mem_sdr [] = "mSDR"; /* memory type */ + static const char mem_ddr [] = "mDDR"; + static const char t_tst [] = "TST"; /* security level */ + static const char t_emu [] = "EMU"; + static const char t_hs [] = "HS"; + static const char t_gp [] = "GP"; + static const char unk [] = "?"; + + const char *cpu_s, *db_s, *mem_s, *sec_s; u32 cpu, rev, sec;
rev = get_cpu_rev(); diff --git a/common/Makefile b/common/Makefile index 5442fbb..4a2027f 100644 --- a/common/Makefile +++ b/common/Makefile @@ -231,6 +231,9 @@ $(obj)env_embedded.o: $(src)env_embedded.c $(obj)../tools/envcrc $(obj)../tools/envcrc: $(MAKE) -C ../tools
+$(obj)hush.o: CFLAGS += -mno-unaligned-access +$(obj)fdt_support.o: CFLAGS += -mno-unaligned-access + #########################################################################
# defines $(obj).depend target diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index 62fb890..01d6b3a 100644 --- a/common/cmd_dfu.c +++ b/common/cmd_dfu.c @@ -30,7 +30,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { const char *str_env; - char s[] = "dfu"; + char *s = "dfu"; char *env_bkp; int ret;
diff --git a/doc/README.arm-unaligned-accesses b/doc/README.arm-unaligned-accesses new file mode 100644 index 0000000..6f07436 --- /dev/null +++ b/doc/README.arm-unaligned-accesses @@ -0,0 +1,110 @@ +If you are reading this because of a data abort: the following MIGHT +be relevant to your abort, if it was caused by an alignment violation. +In order to determine this, use the PC from the abort dump along with +an objdump -s -S of the u-boot ELF binary to locate the function where +the abort happened; then compare this function with the exemples below. +If they match, then you've been hit with a compiler generated unaligned +access, and you should rewrite your code or add -mno-unaligned-access +to the command line of the offending file. + +* + +Since U-Boot runs on a variety of hardware, some only able to perform +unaligned accesses with a strong penalty, some unable to perform them +at all, the policy regarding unaligned accesses is to not perform any, +unless absolutely necessary because of hardware or standards. + +Also, on hardware which permits it, the core is configured to throw +data abort exceptions on unaligned accesses in order to catch these +unallowed accesses as early as possible. + +Until version 4.7, the gcc default for performing unaligned accesses +(-mno-unaligned-access) is to emulate unaligned accesses using aligned +loads and stores plus shifts and masks. Emulated unaligned accesses +will not be caught by hardware. These accesses may be costly and may +be actually unnecessary. In order to catch these accesses and remove +or optimize them, option -munaligned-access is explicitly set for all +versions of gcc which support it. + +From gcc 4.7 onward, the default for performing unaligned accesses +is to use unaligned native loads and stores (-munaligned-access), +because the cost of unaligned accesses has dropped. This should not +affect U-Boot's policy of controlling unaligned accesses, however the +compiler may generate uncontrolled unaligned on its own in at least +one known case: when declaring a local initialized char array, e.g. + +function foo() +{ + char buffer[] = "initial value"; +/* or */ + char buffer[] = { 'i', 'n', 'i', 't', 0 }; + ... +} + +Under -munaligned-accesses with optimizations on, this declaration +causes the compiler to generate native loads from the literal string +and native stores to the buffer, and the literal string alignment +cannot be controlled. If it is misaligned, then the core will throw +a data abort exception. + +Quite probably the same might happen for 16-bit array initializations +where the constant is aligned on a boundary which is a multiple of 2 +but not of 4: + +function foo() +{ + u16 buffer[] = { 1, 2, 3 }; + ... +} + +The long term solution to this issue is to add an option to gcc to +allow controlling the general alignment of data, including constant +initialization values. + +However this will only apply to the version of gcc which will have such +an option. For other versions, there are four workarounds: + +a) Enforce as a rule that array initializations as described above + are forbidden. This is generally not acceptable as they are valid, + and usual, C constructs. The only case where they could be rejected + is when they actually equate to a const char* declaration, i.e. the + array is initialized and never modified in the function's scope. + +b) Drop the requirement on unaligned accesses at least for ARMv7, + i.e. do not throw a data abort exception upon unaligned accesses. + But that will allow adding badly aligned code to U-Boot, only for + it to fail when re-used with another, more strict, target, possibly + once the bad code is already in mainline. + +c) Relax the -munaligned-access rule globally. This will prevent native + unaligned accesses of course, but that will also hide any bug caused + by a bad unaligned access, making it much harder to diagnose it. It + is actually what already happens when building ARM targets with a + pre-4.7 gcc, and it may actually already hide some bugs yet unseen + until the target gets compiled with -munaligned-access. + +d) Relax the -munaligned-access rule only for for files susceptible to + the local initialized array issue. This minimizes the quantity of + code which can hide unwanted misaligned accesses. + +The option retained is d). + +Considering the rarity of actual occurrences (as of this writing, 5 +files out of 7840 in U-Boot, or .3%, contain an initialized local char +array which cannot actually be replaced with a const char*), detection +if the issue in patches should not be asked from contributors. + +Detecting files susceptible to the issue can be automated through a +filter installed as a hook in .git which recognizes local char array +initializations. Automation should err on the false positive side, for +instance flagging non-local arrays as if they were local if they cannot +be told apart. + +In any case, detection shall not prevent committing the patch, but +shall pre-populate the commit message with a note to the effect that +this patch contains an initialized local char or 16-bit array and thus +should be protected from the gcc 4.7 issue. + +Upon a positive detection, either option -mno-unaligned-access should +be applied (if not already) to the affected file(s), or if the array +is a pseudo const char*, it should be replaced by one. diff --git a/fs/fat/Makefile b/fs/fat/Makefile index 9635d36..5c4a2aa 100644 --- a/fs/fat/Makefile +++ b/fs/fat/Makefile @@ -39,6 +39,8 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)file.o: CFLAGS += -mno-unaligned-access
#########################################################################
diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile index ccffe85..71c40f2 100644 --- a/fs/ubifs/Makefile +++ b/fs/ubifs/Makefile @@ -42,6 +42,9 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)super.o: CFLAGS += -mno-unaligned-access + #########################################################################
# defines $(obj).depend target diff --git a/lib/Makefile b/lib/Makefile index a099885..decc5ee 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -83,6 +83,9 @@ OBJS := $(addprefix $(obj),$(COBJS)) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)bzlib.o: CFLAGS += -mno-unaligned-access + #########################################################################
# defines $(obj).depend target
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net --- V2: fix incorrect doc file name in dabort handler message
arch/arm/cpu/arm926ejs/orion5x/cpu.c | 4 +- arch/arm/lib/interrupts.c | 2 +- board/ti/omap2420h4/sys_info.c | 28 ++++----- common/Makefile | 3 + common/cmd_dfu.c | 2 +- doc/README.arm-unaligned-accesses | 110 ++++++++++++++++++++++++++++++++++ fs/fat/Makefile | 2 + fs/ubifs/Makefile | 3 + lib/Makefile | 3 + 9 files changed, 139 insertions(+), 18 deletions(-) create mode 100644 doc/README.arm-unaligned-accesses
diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c b/arch/arm/cpu/arm926ejs/orion5x/cpu.c index c3948d3..5a4775a 100644 --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c @@ -194,8 +194,8 @@ u32 orion5x_device_rev(void) */ int print_cpuinfo(void) { - char dev_str[] = "0x0000"; - char rev_str[] = "0x00"; + char dev_str[7]; /* room enough for 0x0000 plus null byte */ + char rev_str[5]; /* room enough for 0x00 plus null byte */ char *dev_name = NULL; char *rev_name = NULL;
diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c index 74ff5ce..02ccb22 100644 --- a/arch/arm/lib/interrupts.c +++ b/arch/arm/lib/interrupts.c @@ -169,7 +169,7 @@ void do_prefetch_abort (struct pt_regs *pt_regs)
void do_data_abort (struct pt_regs *pt_regs) { - printf ("data abort\n"); + printf ("data abort\n\n MAYBE you should read doc/README.unaligned-accesses\n\n"); show_regs (pt_regs); bad_mode (); } diff --git a/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c index a9f7241..b12011e 100644 --- a/board/ti/omap2420h4/sys_info.c +++ b/board/ti/omap2420h4/sys_info.c @@ -237,20 +237,20 @@ u32 wait_on_value(u32 read_bit_mask, u32 match_value, u32 read_addr, u32 bound) *********************************************************************/ void display_board_info(u32 btype) { - char cpu_2420[] = "2420"; /* cpu type */ - char cpu_2422[] = "2422"; - char cpu_2423[] = "2423"; - char db_men[] = "Menelaus"; /* board type */ - char db_ip[] = "IP"; - char mem_sdr[] = "mSDR"; /* memory type */ - char mem_ddr[] = "mDDR"; - char t_tst[] = "TST"; /* security level */ - char t_emu[] = "EMU"; - char t_hs[] = "HS"; - char t_gp[] = "GP"; - char unk[] = "?"; - - char *cpu_s, *db_s, *mem_s, *sec_s; + static const char cpu_2420 [] = "2420"; /* cpu type */ + static const char cpu_2422 [] = "2422"; + static const char cpu_2423 [] = "2423"; + static const char db_men [] = "Menelaus"; /* board type */ + static const char db_ip [] = "IP"; + static const char mem_sdr [] = "mSDR"; /* memory type */ + static const char mem_ddr [] = "mDDR"; + static const char t_tst [] = "TST"; /* security level */ + static const char t_emu [] = "EMU"; + static const char t_hs [] = "HS"; + static const char t_gp [] = "GP"; + static const char unk [] = "?"; + + const char *cpu_s, *db_s, *mem_s, *sec_s; u32 cpu, rev, sec;
rev = get_cpu_rev(); diff --git a/common/Makefile b/common/Makefile index 973f05a..39dbe16 100644 --- a/common/Makefile +++ b/common/Makefile @@ -232,6 +232,9 @@ $(obj)env_embedded.o: $(src)env_embedded.c $(obj)../tools/envcrc $(obj)../tools/envcrc: $(MAKE) -C ../tools
+$(obj)hush.o: CFLAGS += -mno-unaligned-access +$(obj)fdt_support.o: CFLAGS += -mno-unaligned-access + #########################################################################
# defines $(obj).depend target diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index 62fb890..01d6b3a 100644 --- a/common/cmd_dfu.c +++ b/common/cmd_dfu.c @@ -30,7 +30,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { const char *str_env; - char s[] = "dfu"; + char *s = "dfu"; char *env_bkp; int ret;
diff --git a/doc/README.arm-unaligned-accesses b/doc/README.arm-unaligned-accesses new file mode 100644 index 0000000..6f07436 --- /dev/null +++ b/doc/README.arm-unaligned-accesses @@ -0,0 +1,110 @@ +If you are reading this because of a data abort: the following MIGHT +be relevant to your abort, if it was caused by an alignment violation. +In order to determine this, use the PC from the abort dump along with +an objdump -s -S of the u-boot ELF binary to locate the function where +the abort happened; then compare this function with the exemples below. +If they match, then you've been hit with a compiler generated unaligned +access, and you should rewrite your code or add -mno-unaligned-access +to the command line of the offending file. + +* + +Since U-Boot runs on a variety of hardware, some only able to perform +unaligned accesses with a strong penalty, some unable to perform them +at all, the policy regarding unaligned accesses is to not perform any, +unless absolutely necessary because of hardware or standards. + +Also, on hardware which permits it, the core is configured to throw +data abort exceptions on unaligned accesses in order to catch these +unallowed accesses as early as possible. + +Until version 4.7, the gcc default for performing unaligned accesses +(-mno-unaligned-access) is to emulate unaligned accesses using aligned +loads and stores plus shifts and masks. Emulated unaligned accesses +will not be caught by hardware. These accesses may be costly and may +be actually unnecessary. In order to catch these accesses and remove +or optimize them, option -munaligned-access is explicitly set for all +versions of gcc which support it. + +From gcc 4.7 onward, the default for performing unaligned accesses +is to use unaligned native loads and stores (-munaligned-access), +because the cost of unaligned accesses has dropped. This should not +affect U-Boot's policy of controlling unaligned accesses, however the +compiler may generate uncontrolled unaligned on its own in at least +one known case: when declaring a local initialized char array, e.g. + +function foo() +{ + char buffer[] = "initial value"; +/* or */ + char buffer[] = { 'i', 'n', 'i', 't', 0 }; + ... +} + +Under -munaligned-accesses with optimizations on, this declaration +causes the compiler to generate native loads from the literal string +and native stores to the buffer, and the literal string alignment +cannot be controlled. If it is misaligned, then the core will throw +a data abort exception. + +Quite probably the same might happen for 16-bit array initializations +where the constant is aligned on a boundary which is a multiple of 2 +but not of 4: + +function foo() +{ + u16 buffer[] = { 1, 2, 3 }; + ... +} + +The long term solution to this issue is to add an option to gcc to +allow controlling the general alignment of data, including constant +initialization values. + +However this will only apply to the version of gcc which will have such +an option. For other versions, there are four workarounds: + +a) Enforce as a rule that array initializations as described above + are forbidden. This is generally not acceptable as they are valid, + and usual, C constructs. The only case where they could be rejected + is when they actually equate to a const char* declaration, i.e. the + array is initialized and never modified in the function's scope. + +b) Drop the requirement on unaligned accesses at least for ARMv7, + i.e. do not throw a data abort exception upon unaligned accesses. + But that will allow adding badly aligned code to U-Boot, only for + it to fail when re-used with another, more strict, target, possibly + once the bad code is already in mainline. + +c) Relax the -munaligned-access rule globally. This will prevent native + unaligned accesses of course, but that will also hide any bug caused + by a bad unaligned access, making it much harder to diagnose it. It + is actually what already happens when building ARM targets with a + pre-4.7 gcc, and it may actually already hide some bugs yet unseen + until the target gets compiled with -munaligned-access. + +d) Relax the -munaligned-access rule only for for files susceptible to + the local initialized array issue. This minimizes the quantity of + code which can hide unwanted misaligned accesses. + +The option retained is d). + +Considering the rarity of actual occurrences (as of this writing, 5 +files out of 7840 in U-Boot, or .3%, contain an initialized local char +array which cannot actually be replaced with a const char*), detection +if the issue in patches should not be asked from contributors. + +Detecting files susceptible to the issue can be automated through a +filter installed as a hook in .git which recognizes local char array +initializations. Automation should err on the false positive side, for +instance flagging non-local arrays as if they were local if they cannot +be told apart. + +In any case, detection shall not prevent committing the patch, but +shall pre-populate the commit message with a note to the effect that +this patch contains an initialized local char or 16-bit array and thus +should be protected from the gcc 4.7 issue. + +Upon a positive detection, either option -mno-unaligned-access should +be applied (if not already) to the affected file(s), or if the array +is a pseudo const char*, it should be replaced by one. diff --git a/fs/fat/Makefile b/fs/fat/Makefile index 9635d36..5c4a2aa 100644 --- a/fs/fat/Makefile +++ b/fs/fat/Makefile @@ -39,6 +39,8 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)file.o: CFLAGS += -mno-unaligned-access
#########################################################################
diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile index ccffe85..71c40f2 100644 --- a/fs/ubifs/Makefile +++ b/fs/ubifs/Makefile @@ -42,6 +42,9 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)super.o: CFLAGS += -mno-unaligned-access + #########################################################################
# defines $(obj).depend target diff --git a/lib/Makefile b/lib/Makefile index a099885..decc5ee 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -83,6 +83,9 @@ OBJS := $(addprefix $(obj),$(COBJS)) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)bzlib.o: CFLAGS += -mno-unaligned-access + #########################################################################
# defines $(obj).depend target
Dear Albert Aribaud,
On 05.10.2012 21:15, Albert ARIBAUD wrote:
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net
V2: fix incorrect doc file name in dabort handler message
well, I can not see any change regarding the doc file ;)
arch/arm/cpu/arm926ejs/orion5x/cpu.c | 4 +- arch/arm/lib/interrupts.c | 2 +- board/ti/omap2420h4/sys_info.c | 28 ++++----- common/Makefile | 3 + common/cmd_dfu.c | 2 +- doc/README.arm-unaligned-accesses | 110 ++++++++++++++++++++++++++++++++++
--------------^^
fs/fat/Makefile | 2 + fs/ubifs/Makefile | 3 + lib/Makefile | 3 + 9 files changed, 139 insertions(+), 18 deletions(-) create mode 100644 doc/README.arm-unaligned-accesses
<snip>
diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c index 74ff5ce..02ccb22 100644 --- a/arch/arm/lib/interrupts.c +++ b/arch/arm/lib/interrupts.c @@ -169,7 +169,7 @@ void do_prefetch_abort (struct pt_regs *pt_regs)
void do_data_abort (struct pt_regs *pt_regs) {
- printf ("data abort\n");
- printf ("data abort\n\n MAYBE you should read doc/README.unaligned-accesses\n\n");
--------------------------------------------------------------------^^
show_regs (pt_regs); bad_mode (); }
<snip>
diff --git a/doc/README.arm-unaligned-accesses b/doc/README.arm-unaligned-accesses new file mode 100644 index 0000000..6f07436 --- /dev/null +++ b/doc/README.arm-unaligned-accesses @@ -0,0 +1,110 @@ +If you are reading this because of a data abort: the following MIGHT +be relevant to your abort, if it was caused by an alignment violation. +In order to determine this, use the PC from the abort dump along with +an objdump -s -S of the u-boot ELF binary to locate the function where +the abort happened; then compare this function with the exemples below.
examples
+If they match, then you've been hit with a compiler generated unaligned +access, and you should rewrite your code or add -mno-unaligned-access +to the command line of the offending file.
+*
+Since U-Boot runs on a variety of hardware, some only able to perform +unaligned accesses with a strong penalty, some unable to perform them +at all, the policy regarding unaligned accesses is to not perform any, +unless absolutely necessary because of hardware or standards.
+Also, on hardware which permits it, the core is configured to throw +data abort exceptions on unaligned accesses in order to catch these
----------------------------------------------^^
+unallowed accesses as early as possible.
+Until version 4.7, the gcc default for performing unaligned accesses +(-mno-unaligned-access) is to emulate unaligned accesses using aligned +loads and stores plus shifts and masks. Emulated unaligned accesses +will not be caught by hardware. These accesses may be costly and may +be actually unnecessary. In order to catch these accesses and remove
-----^^
+or optimize them, option -munaligned-access is explicitly set for all +versions of gcc which support it.
+From gcc 4.7 onward, the default for performing unaligned accesses +is to use unaligned native loads and stores (-munaligned-access), +because the cost of unaligned accesses has dropped. This should not +affect U-Boot's policy of controlling unaligned accesses, however the +compiler may generate uncontrolled unaligned on its own in at least
-----------------------------------------------^ access?
+one known case: when declaring a local initialized char array, e.g.
+function foo() +{
- char buffer[] = "initial value";
+/* or */
- char buffer[] = { 'i', 'n', 'i', 't', 0 };
- ...
+}
+Under -munaligned-accesses with optimizations on, this declaration +causes the compiler to generate native loads from the literal string +and native stores to the buffer, and the literal string alignment +cannot be controlled. If it is misaligned, then the core will throw +a data abort exception.
+Quite probably the same might happen for 16-bit array initializations +where the constant is aligned on a boundary which is a multiple of 2 +but not of 4:
+function foo() +{
- u16 buffer[] = { 1, 2, 3 };
- ...
+}
+The long term solution to this issue is to add an option to gcc to +allow controlling the general alignment of data, including constant +initialization values.
+However this will only apply to the version of gcc which will have such +an option. For other versions, there are four workarounds:
+a) Enforce as a rule that array initializations as described above
- are forbidden. This is generally not acceptable as they are valid,
- and usual, C constructs. The only case where they could be rejected
- is when they actually equate to a const char* declaration, i.e. the
- array is initialized and never modified in the function's scope.
+b) Drop the requirement on unaligned accesses at least for ARMv7,
- i.e. do not throw a data abort exception upon unaligned accesses.
- But that will allow adding badly aligned code to U-Boot, only for
- it to fail when re-used with another, more strict, target, possibly
-------------------------------------------------------^
- once the bad code is already in mainline.
+c) Relax the -munaligned-access rule globally. This will prevent native
- unaligned accesses of course, but that will also hide any bug caused
- by a bad unaligned access, making it much harder to diagnose it. It
- is actually what already happens when building ARM targets with a
- pre-4.7 gcc, and it may actually already hide some bugs yet unseen
- until the target gets compiled with -munaligned-access.
+d) Relax the -munaligned-access rule only for for files susceptible to
- the local initialized array issue. This minimizes the quantity of
- code which can hide unwanted misaligned accesses.
+The option retained is d).
+Considering the rarity of actual occurrences (as of this writing, 5 +files out of 7840 in U-Boot, or .3%, contain an initialized local char +array which cannot actually be replaced with a const char*), detection +if the issue in patches should not be asked from contributors.
---8<--- Considering the rarity of actual occurrences, detection if the issue in patches should not be asked from contributors. --->8---
I think there is something wrong wih this sentence ... albeit I can not definitely say what it is.
+Detecting files susceptible to the issue can be automated through a +filter installed as a hook in .git which recognizes local char array +initializations. Automation should err on the false positive side, for +instance flagging non-local arrays as if they were local if they cannot +be told apart.
Isn't that a suggestion that should be asked to the list instead? I wonder why it is written down in this README document.
+In any case, detection shall not prevent committing the patch, but +shall pre-populate the commit message with a note to the effect that +this patch contains an initialized local char or 16-bit array and thus +should be protected from the gcc 4.7 issue.
+Upon a positive detection, either option -mno-unaligned-access should +be applied (if not already) to the affected file(s), or if the array +is a pseudo const char*, it should be replaced by one.
Best regards
Andreas Bießmann
Hi Andreas,
On Mon, 08 Oct 2012 13:21:45 +0200, "Andreas Bießmann" andreas.devel@googlemail.com wrote:
Dear Albert Aribaud,
On 05.10.2012 21:15, Albert ARIBAUD wrote:
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net
V2: fix incorrect doc file name in dabort handler message
well, I can not see any change regarding the doc file ;)
Maybe you though there was a change in the doc file itself? V2 is about correcting the file *name* as printed out by the dabort handler.
arch/arm/cpu/arm926ejs/orion5x/cpu.c | 4 +- arch/arm/lib/interrupts.c | 2 +- board/ti/omap2420h4/sys_info.c | 28 ++++----- common/Makefile | 3 + common/cmd_dfu.c | 2 +- doc/README.arm-unaligned-accesses | 110 ++++++++++++++++++++++++++++++++++
--------------^^
fs/fat/Makefile | 2 + fs/ubifs/Makefile | 3 + lib/Makefile | 3 + 9 files changed, 139 insertions(+), 18 deletions(-) create mode 100644 doc/README.arm-unaligned-accesses
<snip>
diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c index 74ff5ce..02ccb22 100644 --- a/arch/arm/lib/interrupts.c +++ b/arch/arm/lib/interrupts.c @@ -169,7 +169,7 @@ void do_prefetch_abort (struct pt_regs *pt_regs)
void do_data_abort (struct pt_regs *pt_regs) {
- printf ("data abort\n");
- printf ("data abort\n\n MAYBE you should read doc/README.unaligned-accesses\n\n");
--------------------------------------------------------------------^^
show_regs (pt_regs); bad_mode (); }
<snip>
diff --git a/doc/README.arm-unaligned-accesses b/doc/README.arm-unaligned-accesses new file mode 100644 index 0000000..6f07436 --- /dev/null +++ b/doc/README.arm-unaligned-accesses @@ -0,0 +1,110 @@ +If you are reading this because of a data abort: the following MIGHT +be relevant to your abort, if it was caused by an alignment violation. +In order to determine this, use the PC from the abort dump along with +an objdump -s -S of the u-boot ELF binary to locate the function where +the abort happened; then compare this function with the exemples below.
examples
That's the French touch. :)
Will fix in V3.
+If they match, then you've been hit with a compiler generated unaligned +access, and you should rewrite your code or add -mno-unaligned-access +to the command line of the offending file.
+*
+Since U-Boot runs on a variety of hardware, some only able to perform +unaligned accesses with a strong penalty, some unable to perform them +at all, the policy regarding unaligned accesses is to not perform any, +unless absolutely necessary because of hardware or standards.
+Also, on hardware which permits it, the core is configured to throw +data abort exceptions on unaligned accesses in order to catch these
----------------------------------------------^^
Will remove all extra spaces in doc.
+unallowed accesses as early as possible.
+Until version 4.7, the gcc default for performing unaligned accesses +(-mno-unaligned-access) is to emulate unaligned accesses using aligned +loads and stores plus shifts and masks. Emulated unaligned accesses +will not be caught by hardware. These accesses may be costly and may +be actually unnecessary. In order to catch these accesses and remove
-----^^
+or optimize them, option -munaligned-access is explicitly set for all +versions of gcc which support it.
+From gcc 4.7 onward, the default for performing unaligned accesses +is to use unaligned native loads and stores (-munaligned-access), +because the cost of unaligned accesses has dropped. This should not +affect U-Boot's policy of controlling unaligned accesses, however the +compiler may generate uncontrolled unaligned on its own in at least
-----------------------------------------------^ access?
"AccessES", even. Will fix.
+one known case: when declaring a local initialized char array, e.g.
+function foo() +{
- char buffer[] = "initial value";
+/* or */
- char buffer[] = { 'i', 'n', 'i', 't', 0 };
- ...
+}
+Under -munaligned-accesses with optimizations on, this declaration +causes the compiler to generate native loads from the literal string +and native stores to the buffer, and the literal string alignment +cannot be controlled. If it is misaligned, then the core will throw +a data abort exception.
+Quite probably the same might happen for 16-bit array initializations +where the constant is aligned on a boundary which is a multiple of 2 +but not of 4:
+function foo() +{
- u16 buffer[] = { 1, 2, 3 };
- ...
+}
+The long term solution to this issue is to add an option to gcc to +allow controlling the general alignment of data, including constant +initialization values.
+However this will only apply to the version of gcc which will have such +an option. For other versions, there are four workarounds:
+a) Enforce as a rule that array initializations as described above
- are forbidden. This is generally not acceptable as they are valid,
- and usual, C constructs. The only case where they could be rejected
- is when they actually equate to a const char* declaration, i.e. the
- array is initialized and never modified in the function's scope.
+b) Drop the requirement on unaligned accesses at least for ARMv7,
- i.e. do not throw a data abort exception upon unaligned accesses.
- But that will allow adding badly aligned code to U-Boot, only for
- it to fail when re-used with another, more strict, target, possibly
-------------------------------------------------------^
- once the bad code is already in mainline.
+c) Relax the -munaligned-access rule globally. This will prevent native
- unaligned accesses of course, but that will also hide any bug caused
- by a bad unaligned access, making it much harder to diagnose it. It
- is actually what already happens when building ARM targets with a
- pre-4.7 gcc, and it may actually already hide some bugs yet unseen
- until the target gets compiled with -munaligned-access.
+d) Relax the -munaligned-access rule only for for files susceptible to
- the local initialized array issue. This minimizes the quantity of
- code which can hide unwanted misaligned accesses.
+The option retained is d).
+Considering the rarity of actual occurrences (as of this writing, 5 +files out of 7840 in U-Boot, or .3%, contain an initialized local char +array which cannot actually be replaced with a const char*), detection +if the issue in patches should not be asked from contributors.
---8<--- Considering the rarity of actual occurrences, detection if the issue in patches should not be asked from contributors. --->8---
I think there is something wrong wih this sentence ... albeit I can not definitely say what it is.
s/if/of/, but yes, the sentence could use some rewriting. "Considering that actual occurrences of the issue are rare, contributors should not be required to systematically try and detect the issue in their patches". Would that be ok?
+Detecting files susceptible to the issue can be automated through a +filter installed as a hook in .git which recognizes local char array +initializations. Automation should err on the false positive side, for +instance flagging non-local arrays as if they were local if they cannot +be told apart.
Isn't that a suggestion that should be asked to the list instead? I wonder why it is written down in this README document.
Not sure I understand what you mean about "ask[ing] the list instead".
As for writing it down, I did because it's better written down than forgotten, and at this point there is no proper way to auto install a git hook in a U-Boot developer's tree, at least none that I would feel safe pushing so close to a release. So even if I did provide a script, each developer would have to manually put the hook in place. Therefore, the doc just says how to automate detection.
+In any case, detection shall not prevent committing the patch, but +shall pre-populate the commit message with a note to the effect that +this patch contains an initialized local char or 16-bit array and thus +should be protected from the gcc 4.7 issue.
+Upon a positive detection, either option -mno-unaligned-access should +be applied (if not already) to the affected file(s), or if the array +is a pseudo const char*, it should be replaced by one.
Best regards
Andreas Bießmann
Thanks a lot for your feedback!
Amicalement,
Hi Albert,
On 08.10.2012 16:36, Albert ARIBAUD wrote:
Hi Andreas,
On Mon, 08 Oct 2012 13:21:45 +0200, "Andreas Bießmann" andreas.devel@googlemail.com wrote:
Dear Albert Aribaud,
On 05.10.2012 21:15, Albert ARIBAUD wrote:
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net
V2: fix incorrect doc file name in dabort handler message
well, I can not see any change regarding the doc file ;)
Maybe you though there was a change in the doc file itself? V2 is about correcting the file *name* as printed out by the dabort handler.
Well, that was I understood by the V2 comment ...
but V1 has: ---8<--- - printf ("data abort\n"); + printf ("data abort\n\n MAYBE you should read doc/README.unaligned-access\n\n"); --->8---
and V2 has: ---8<--- - printf ("data abort\n"); + printf ("data abort\n\n MAYBE you should read doc/README.unaligned-accesses\n\n"); --->8---
However the filename is 'doc/README.arm-unaligned-accesses'
<snip>
+Considering the rarity of actual occurrences (as of this writing, 5 +files out of 7840 in U-Boot, or .3%, contain an initialized local char +array which cannot actually be replaced with a const char*), detection +if the issue in patches should not be asked from contributors.
---8<--- Considering the rarity of actual occurrences, detection if the issue in patches should not be asked from contributors. --->8---
I think there is something wrong wih this sentence ... albeit I can not definitely say what it is.
s/if/of/, but yes, the sentence could use some rewriting. "Considering that actual occurrences of the issue are rare, contributors should not be required to systematically try and detect the issue in their patches". Would that be ok?
Sounds way better.
+Detecting files susceptible to the issue can be automated through a +filter installed as a hook in .git which recognizes local char array +initializations. Automation should err on the false positive side, for +instance flagging non-local arrays as if they were local if they cannot +be told apart.
Isn't that a suggestion that should be asked to the list instead? I wonder why it is written down in this README document.
Not sure I understand what you mean about "ask[ing] the list instead".
I meant to discuss this on the list and just install the hook then (at least for the denx.de repositories).
As for writing it down, I did because it's better written down than forgotten, and at this point there is no proper way to auto install a git hook in a U-Boot developer's tree, at least none that I would feel safe pushing so close to a release. So even if I did provide a script, each developer would have to manually put the hook in place. Therefore, the doc just says how to automate detection.
But you are right, so forget my comment.
best regards
Andreas Bießmann
Hi Andreas,
On Mon, 08 Oct 2012 16:56:41 +0200, "Andreas Bießmann" andreas.devel@googlemail.com wrote:
Hi Albert,
On 08.10.2012 16:36, Albert ARIBAUD wrote:
Hi Andreas,
On Mon, 08 Oct 2012 13:21:45 +0200, "Andreas Bießmann" andreas.devel@googlemail.com wrote:
Dear Albert Aribaud,
On 05.10.2012 21:15, Albert ARIBAUD wrote:
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net
V2: fix incorrect doc file name in dabort handler message
well, I can not see any change regarding the doc file ;)
Maybe you though there was a change in the doc file itself? V2 is about correcting the file *name* as printed out by the dabort handler.
Well, that was I understood by the V2 comment ...
but V1 has: ---8<---
- printf ("data abort\n");
- printf ("data abort\n\n MAYBE you should read
doc/README.unaligned-access\n\n"); --->8---
and V2 has: ---8<---
- printf ("data abort\n");
- printf ("data abort\n\n MAYBE you should read
doc/README.unaligned-accesses\n\n"); --->8---
However the filename is 'doc/README.arm-unaligned-accesses'
Argh!
Will fix in V3... For good. Thanks.
<snip>
+Considering the rarity of actual occurrences (as of this writing, 5 +files out of 7840 in U-Boot, or .3%, contain an initialized local char +array which cannot actually be replaced with a const char*), detection +if the issue in patches should not be asked from contributors.
---8<--- Considering the rarity of actual occurrences, detection if the issue in patches should not be asked from contributors. --->8---
I think there is something wrong wih this sentence ... albeit I can not definitely say what it is.
s/if/of/, but yes, the sentence could use some rewriting. "Considering that actual occurrences of the issue are rare, contributors should not be required to systematically try and detect the issue in their patches". Would that be ok?
Sounds way better.
Will put in V3.
+Detecting files susceptible to the issue can be automated through a +filter installed as a hook in .git which recognizes local char array +initializations. Automation should err on the false positive side, for +instance flagging non-local arrays as if they were local if they cannot +be told apart.
Isn't that a suggestion that should be asked to the list instead? I wonder why it is written down in this README document.
Not sure I understand what you mean about "ask[ing] the list instead".
I meant to discuss this on the list and just install the hook then (at least for the denx.de repositories).
As for writing it down, I did because it's better written down than forgotten, and at this point there is no proper way to auto install a git hook in a U-Boot developer's tree, at least none that I would feel safe pushing so close to a release. So even if I did provide a script, each developer would have to manually put the hook in place. Therefore, the doc just says how to automate detection.
But you are right, so forget my comment.
Thanks.
best regards
Andreas Bießmann
Amicalement,
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net --- V3: *really* fix incorrect doc file name in dabort handler message clarifications and typo fixes in README.arm-unaligned-accesses V2: fix incorrect doc file name in dabort handler message
arch/arm/cpu/arm926ejs/orion5x/cpu.c | 4 +- arch/arm/lib/interrupts.c | 2 +- board/ti/omap2420h4/sys_info.c | 28 ++++----- common/Makefile | 3 + common/cmd_dfu.c | 2 +- doc/README.arm-unaligned-accesses | 112 ++++++++++++++++++++++++++++++++++ fs/fat/Makefile | 2 + fs/ubifs/Makefile | 3 + lib/Makefile | 3 + 9 files changed, 141 insertions(+), 18 deletions(-) create mode 100644 doc/README.arm-unaligned-accesses
diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c b/arch/arm/cpu/arm926ejs/orion5x/cpu.c index c3948d3..5a4775a 100644 --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c @@ -194,8 +194,8 @@ u32 orion5x_device_rev(void) */ int print_cpuinfo(void) { - char dev_str[] = "0x0000"; - char rev_str[] = "0x00"; + char dev_str[7]; /* room enough for 0x0000 plus null byte */ + char rev_str[5]; /* room enough for 0x00 plus null byte */ char *dev_name = NULL; char *rev_name = NULL;
diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c index 74ff5ce..02124a7 100644 --- a/arch/arm/lib/interrupts.c +++ b/arch/arm/lib/interrupts.c @@ -169,7 +169,7 @@ void do_prefetch_abort (struct pt_regs *pt_regs)
void do_data_abort (struct pt_regs *pt_regs) { - printf ("data abort\n"); + printf ("data abort\n\n MAYBE you should read doc/README.arm-unaligned-accesses\n\n"); show_regs (pt_regs); bad_mode (); } diff --git a/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c index a9f7241..b12011e 100644 --- a/board/ti/omap2420h4/sys_info.c +++ b/board/ti/omap2420h4/sys_info.c @@ -237,20 +237,20 @@ u32 wait_on_value(u32 read_bit_mask, u32 match_value, u32 read_addr, u32 bound) *********************************************************************/ void display_board_info(u32 btype) { - char cpu_2420[] = "2420"; /* cpu type */ - char cpu_2422[] = "2422"; - char cpu_2423[] = "2423"; - char db_men[] = "Menelaus"; /* board type */ - char db_ip[] = "IP"; - char mem_sdr[] = "mSDR"; /* memory type */ - char mem_ddr[] = "mDDR"; - char t_tst[] = "TST"; /* security level */ - char t_emu[] = "EMU"; - char t_hs[] = "HS"; - char t_gp[] = "GP"; - char unk[] = "?"; - - char *cpu_s, *db_s, *mem_s, *sec_s; + static const char cpu_2420 [] = "2420"; /* cpu type */ + static const char cpu_2422 [] = "2422"; + static const char cpu_2423 [] = "2423"; + static const char db_men [] = "Menelaus"; /* board type */ + static const char db_ip [] = "IP"; + static const char mem_sdr [] = "mSDR"; /* memory type */ + static const char mem_ddr [] = "mDDR"; + static const char t_tst [] = "TST"; /* security level */ + static const char t_emu [] = "EMU"; + static const char t_hs [] = "HS"; + static const char t_gp [] = "GP"; + static const char unk [] = "?"; + + const char *cpu_s, *db_s, *mem_s, *sec_s; u32 cpu, rev, sec;
rev = get_cpu_rev(); diff --git a/common/Makefile b/common/Makefile index 973f05a..39dbe16 100644 --- a/common/Makefile +++ b/common/Makefile @@ -232,6 +232,9 @@ $(obj)env_embedded.o: $(src)env_embedded.c $(obj)../tools/envcrc $(obj)../tools/envcrc: $(MAKE) -C ../tools
+$(obj)hush.o: CFLAGS += -mno-unaligned-access +$(obj)fdt_support.o: CFLAGS += -mno-unaligned-access + #########################################################################
# defines $(obj).depend target diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index 62fb890..01d6b3a 100644 --- a/common/cmd_dfu.c +++ b/common/cmd_dfu.c @@ -30,7 +30,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { const char *str_env; - char s[] = "dfu"; + char *s = "dfu"; char *env_bkp; int ret;
diff --git a/doc/README.arm-unaligned-accesses b/doc/README.arm-unaligned-accesses new file mode 100644 index 0000000..35119ec --- /dev/null +++ b/doc/README.arm-unaligned-accesses @@ -0,0 +1,112 @@ +If you are reading this because of a data abort: the following MIGHT +be relevant to your abort, if it was caused by an alignment violation. +In order to determine this, use the PC from the abort dump along with +an objdump -s -S of the u-boot ELF binary to locate the function where +the abort happened; then compare this function with the examples below. +If they match, then you've been hit with a compiler generated unaligned +access, and you should rewrite your code or add -mno-unaligned-access +to the command line of the offending file. + +* + +Since U-Boot runs on a variety of hardware, some only able to perform +unaligned accesses with a strong penalty, some unable to perform them +at all, the policy regarding unaligned accesses is to not perform any, +unless absolutely necessary because of hardware or standards. + +Also, on hardware which permits it, the core is configured to throw +data abort exceptions on unaligned accesses in order to catch these +unallowed accesses as early as possible. + +Until version 4.7, the gcc default for performing unaligned accesses +(-mno-unaligned-access) is to emulate unaligned accesses using aligned +loads and stores plus shifts and masks. Emulated unaligned accesses +will not be caught by hardware. These accesses may be costly and may +be actually unnecessary. In order to catch these accesses and remove +or optimize them, option -munaligned-access is explicitly set for all +versions of gcc which support it. + +From gcc 4.7 onward, the default for performing unaligned accesses +is to use unaligned native loads and stores (-munaligned-access), +because the cost of unaligned accesses has dropped. This should not +affect U-Boot's policy of controlling unaligned accesses, however the +compiler may generate uncontrolled unaligned accesses on its own in at +least one known case: when declaring a local initialized char array, +e.g. + +function foo() +{ + char buffer[] = "initial value"; +/* or */ + char buffer[] = { 'i', 'n', 'i', 't', 0 }; + ... +} + +Under -munaligned-accesses with optimizations on, this declaration +causes the compiler to generate native loads from the literal string +and native stores to the buffer, and the literal string alignment +cannot be controlled. If it is misaligned, then the core will throw +a data abort exception. + +Quite probably the same might happen for 16-bit array initializations +where the constant is aligned on a boundary which is a multiple of 2 +but not of 4: + +function foo() +{ + u16 buffer[] = { 1, 2, 3 }; + ... +} + +The long term solution to this issue is to add an option to gcc to +allow controlling the general alignment of data, including constant +initialization values. + +However this will only apply to the version of gcc which will have such +an option. For other versions, there are four workarounds: + +a) Enforce as a rule that array initializations as described above + are forbidden. This is generally not acceptable as they are valid, + and usual, C constructs. The only case where they could be rejected + is when they actually equate to a const char* declaration, i.e. the + array is initialized and never modified in the function's scope. + +b) Drop the requirement on unaligned accesses at least for ARMv7, + i.e. do not throw a data abort exception upon unaligned accesses. + But that will allow adding badly aligned code to U-Boot, only for + it to fail when re-used with a stricter target, possibly once the + bad code is already in mainline. + +c) Relax the -munaligned-access rule globally. This will prevent native + unaligned accesses of course, but that will also hide any bug caused + by a bad unaligned access, making it much harder to diagnose it. It + is actually what already happens when building ARM targets with a + pre-4.7 gcc, and it may actually already hide some bugs yet unseen + until the target gets compiled with -munaligned-access. + +d) Relax the -munaligned-access rule only for for files susceptible to + the local initialized array issue. This minimizes the quantity of + code which can hide unwanted misaligned accesses. + +The option retained is d). + +Considering that actual occurrences of the issue are rare (as of this +writing, 5 files out of 7840 in U-Boot, or .3%, contain an initialized +local char array which cannot actually be replaced with a const char*), +contributors should not be required to systematically try and detect +the issue in their patches. + +Detecting files susceptible to the issue can be automated through a +filter installed as a hook in .git which recognizes local char array +initializations. Automation should err on the false positive side, for +instance flagging non-local arrays as if they were local if they cannot +be told apart. + +In any case, detection shall not prevent committing the patch, but +shall pre-populate the commit message with a note to the effect that +this patch contains an initialized local char or 16-bit array and thus +should be protected from the gcc 4.7 issue. + +Upon a positive detection, either option -mno-unaligned-access should +be applied (if not already) to the affected file(s), or if the array +is a pseudo const char*, it should be replaced by one. diff --git a/fs/fat/Makefile b/fs/fat/Makefile index 9635d36..5c4a2aa 100644 --- a/fs/fat/Makefile +++ b/fs/fat/Makefile @@ -39,6 +39,8 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)file.o: CFLAGS += -mno-unaligned-access
#########################################################################
diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile index ccffe85..71c40f2 100644 --- a/fs/ubifs/Makefile +++ b/fs/ubifs/Makefile @@ -42,6 +42,9 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)super.o: CFLAGS += -mno-unaligned-access + #########################################################################
# defines $(obj).depend target diff --git a/lib/Makefile b/lib/Makefile index a099885..decc5ee 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -83,6 +83,9 @@ OBJS := $(addprefix $(obj),$(COBJS)) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)bzlib.o: CFLAGS += -mno-unaligned-access + #########################################################################
# defines $(obj).depend target
On Mon, Oct 08, 2012 at 09:19:04PM +0200, Albert ARIBAUD wrote:
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net
[snip]
diff --git a/common/Makefile b/common/Makefile index 973f05a..39dbe16 100644 --- a/common/Makefile +++ b/common/Makefile @@ -232,6 +232,9 @@ $(obj)env_embedded.o: $(src)env_embedded.c $(obj)../tools/envcrc $(obj)../tools/envcrc: $(MAKE) -C ../tools
+$(obj)hush.o: CFLAGS += -mno-unaligned-access +$(obj)fdt_support.o: CFLAGS += -mno-unaligned-access
Forgot a '# SEE doc/...' here.
[snip]
+If you are reading this because of a data abort: the following MIGHT +be relevant to your abort, if it was caused by an alignment violation. +In order to determine this, use the PC from the abort dump along with +an objdump -s -S of the u-boot ELF binary to locate the function where +the abort happened; then compare this function with the examples below.
You should also add something about how if you are able to reach the U-Boot prompt before the abort happens, if CONFIG_CMD_BDI is set the 'bdinfo' command will give you the relocation offset to apply to the PC.
Otherwise looks good. Thanks!
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net --- V4: added information on how to find relocation offset for pc common/Makefile missed a comment re README.arm-unaligned-accesses V3: *really* fix incorrect doc file name in dabort handler message clarifications and typo fixes in README.arm-unaligned-accesses V2: fix incorrect doc file name in dabort handler message
arch/arm/cpu/arm926ejs/orion5x/cpu.c | 4 +- arch/arm/lib/interrupts.c | 2 +- board/ti/omap2420h4/sys_info.c | 28 ++++---- common/Makefile | 4 ++ common/cmd_dfu.c | 2 +- doc/README.arm-unaligned-accesses | 121 ++++++++++++++++++++++++++++++++++ fs/fat/Makefile | 2 + fs/ubifs/Makefile | 3 + lib/Makefile | 3 + 9 files changed, 151 insertions(+), 18 deletions(-) create mode 100644 doc/README.arm-unaligned-accesses
diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c b/arch/arm/cpu/arm926ejs/orion5x/cpu.c index c3948d3..5a4775a 100644 --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c @@ -194,8 +194,8 @@ u32 orion5x_device_rev(void) */ int print_cpuinfo(void) { - char dev_str[] = "0x0000"; - char rev_str[] = "0x00"; + char dev_str[7]; /* room enough for 0x0000 plus null byte */ + char rev_str[5]; /* room enough for 0x00 plus null byte */ char *dev_name = NULL; char *rev_name = NULL;
diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c index 74ff5ce..02124a7 100644 --- a/arch/arm/lib/interrupts.c +++ b/arch/arm/lib/interrupts.c @@ -169,7 +169,7 @@ void do_prefetch_abort (struct pt_regs *pt_regs)
void do_data_abort (struct pt_regs *pt_regs) { - printf ("data abort\n"); + printf ("data abort\n\n MAYBE you should read doc/README.arm-unaligned-accesses\n\n"); show_regs (pt_regs); bad_mode (); } diff --git a/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c index a9f7241..b12011e 100644 --- a/board/ti/omap2420h4/sys_info.c +++ b/board/ti/omap2420h4/sys_info.c @@ -237,20 +237,20 @@ u32 wait_on_value(u32 read_bit_mask, u32 match_value, u32 read_addr, u32 bound) *********************************************************************/ void display_board_info(u32 btype) { - char cpu_2420[] = "2420"; /* cpu type */ - char cpu_2422[] = "2422"; - char cpu_2423[] = "2423"; - char db_men[] = "Menelaus"; /* board type */ - char db_ip[] = "IP"; - char mem_sdr[] = "mSDR"; /* memory type */ - char mem_ddr[] = "mDDR"; - char t_tst[] = "TST"; /* security level */ - char t_emu[] = "EMU"; - char t_hs[] = "HS"; - char t_gp[] = "GP"; - char unk[] = "?"; - - char *cpu_s, *db_s, *mem_s, *sec_s; + static const char cpu_2420 [] = "2420"; /* cpu type */ + static const char cpu_2422 [] = "2422"; + static const char cpu_2423 [] = "2423"; + static const char db_men [] = "Menelaus"; /* board type */ + static const char db_ip [] = "IP"; + static const char mem_sdr [] = "mSDR"; /* memory type */ + static const char mem_ddr [] = "mDDR"; + static const char t_tst [] = "TST"; /* security level */ + static const char t_emu [] = "EMU"; + static const char t_hs [] = "HS"; + static const char t_gp [] = "GP"; + static const char unk [] = "?"; + + const char *cpu_s, *db_s, *mem_s, *sec_s; u32 cpu, rev, sec;
rev = get_cpu_rev(); diff --git a/common/Makefile b/common/Makefile index 973f05a..a498367 100644 --- a/common/Makefile +++ b/common/Makefile @@ -232,6 +232,10 @@ $(obj)env_embedded.o: $(src)env_embedded.c $(obj)../tools/envcrc $(obj)../tools/envcrc: $(MAKE) -C ../tools
+# SEE README.arm-unaligned-accesses +$(obj)hush.o: CFLAGS += -mno-unaligned-access +$(obj)fdt_support.o: CFLAGS += -mno-unaligned-access + #########################################################################
# defines $(obj).depend target diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index 62fb890..01d6b3a 100644 --- a/common/cmd_dfu.c +++ b/common/cmd_dfu.c @@ -30,7 +30,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { const char *str_env; - char s[] = "dfu"; + char *s = "dfu"; char *env_bkp; int ret;
diff --git a/doc/README.arm-unaligned-accesses b/doc/README.arm-unaligned-accesses new file mode 100644 index 0000000..08a70c0 --- /dev/null +++ b/doc/README.arm-unaligned-accesses @@ -0,0 +1,121 @@ +If you are reading this because of a data abort: the following MIGHT +be relevant to your abort, if it was caused by an alignment violation. +In order to determine this, use the PC from the abort dump along with +an objdump -s -S of the u-boot ELF binary to locate the function where +the abort happened; then compare this function with the examples below. +If they match, then you've been hit with a compiler generated unaligned +access, and you should rewrite your code or add -mno-unaligned-access +to the command line of the offending file. + +Note that the PC shown in the abort message is relocated. In order to +be able to match it to an address in the ELF binary dump, you will need +to know the relocation offset. If your target defines CONFIG_CMD_BDI +and if you can get to the prompt and enter commands before the abort +happens, then command "bdinfo" will give you the offset. Otherwise you +will need to try a build with DEBUG set, which will display the offset, +or use a debugger and set a breakpoint at relocate_code() to see the +offset (passed as an argument). + +* + +Since U-Boot runs on a variety of hardware, some only able to perform +unaligned accesses with a strong penalty, some unable to perform them +at all, the policy regarding unaligned accesses is to not perform any, +unless absolutely necessary because of hardware or standards. + +Also, on hardware which permits it, the core is configured to throw +data abort exceptions on unaligned accesses in order to catch these +unallowed accesses as early as possible. + +Until version 4.7, the gcc default for performing unaligned accesses +(-mno-unaligned-access) is to emulate unaligned accesses using aligned +loads and stores plus shifts and masks. Emulated unaligned accesses +will not be caught by hardware. These accesses may be costly and may +be actually unnecessary. In order to catch these accesses and remove +or optimize them, option -munaligned-access is explicitly set for all +versions of gcc which support it. + +From gcc 4.7 onward, the default for performing unaligned accesses +is to use unaligned native loads and stores (-munaligned-access), +because the cost of unaligned accesses has dropped. This should not +affect U-Boot's policy of controlling unaligned accesses, however the +compiler may generate uncontrolled unaligned accesses on its own in at +least one known case: when declaring a local initialized char array, +e.g. + +function foo() +{ + char buffer[] = "initial value"; +/* or */ + char buffer[] = { 'i', 'n', 'i', 't', 0 }; + ... +} + +Under -munaligned-accesses with optimizations on, this declaration +causes the compiler to generate native loads from the literal string +and native stores to the buffer, and the literal string alignment +cannot be controlled. If it is misaligned, then the core will throw +a data abort exception. + +Quite probably the same might happen for 16-bit array initializations +where the constant is aligned on a boundary which is a multiple of 2 +but not of 4: + +function foo() +{ + u16 buffer[] = { 1, 2, 3 }; + ... +} + +The long term solution to this issue is to add an option to gcc to +allow controlling the general alignment of data, including constant +initialization values. + +However this will only apply to the version of gcc which will have such +an option. For other versions, there are four workarounds: + +a) Enforce as a rule that array initializations as described above + are forbidden. This is generally not acceptable as they are valid, + and usual, C constructs. The only case where they could be rejected + is when they actually equate to a const char* declaration, i.e. the + array is initialized and never modified in the function's scope. + +b) Drop the requirement on unaligned accesses at least for ARMv7, + i.e. do not throw a data abort exception upon unaligned accesses. + But that will allow adding badly aligned code to U-Boot, only for + it to fail when re-used with a stricter target, possibly once the + bad code is already in mainline. + +c) Relax the -munaligned-access rule globally. This will prevent native + unaligned accesses of course, but that will also hide any bug caused + by a bad unaligned access, making it much harder to diagnose it. It + is actually what already happens when building ARM targets with a + pre-4.7 gcc, and it may actually already hide some bugs yet unseen + until the target gets compiled with -munaligned-access. + +d) Relax the -munaligned-access rule only for for files susceptible to + the local initialized array issue. This minimizes the quantity of + code which can hide unwanted misaligned accesses. + +The option retained is d). + +Considering that actual occurrences of the issue are rare (as of this +writing, 5 files out of 7840 in U-Boot, or .3%, contain an initialized +local char array which cannot actually be replaced with a const char*), +contributors should not be required to systematically try and detect +the issue in their patches. + +Detecting files susceptible to the issue can be automated through a +filter installed as a hook in .git which recognizes local char array +initializations. Automation should err on the false positive side, for +instance flagging non-local arrays as if they were local if they cannot +be told apart. + +In any case, detection shall not prevent committing the patch, but +shall pre-populate the commit message with a note to the effect that +this patch contains an initialized local char or 16-bit array and thus +should be protected from the gcc 4.7 issue. + +Upon a positive detection, either option -mno-unaligned-access should +be applied (if not already) to the affected file(s), or if the array +is a pseudo const char*, it should be replaced by one. diff --git a/fs/fat/Makefile b/fs/fat/Makefile index 9635d36..5c4a2aa 100644 --- a/fs/fat/Makefile +++ b/fs/fat/Makefile @@ -39,6 +39,8 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)file.o: CFLAGS += -mno-unaligned-access
#########################################################################
diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile index ccffe85..71c40f2 100644 --- a/fs/ubifs/Makefile +++ b/fs/ubifs/Makefile @@ -42,6 +42,9 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)super.o: CFLAGS += -mno-unaligned-access + #########################################################################
# defines $(obj).depend target diff --git a/lib/Makefile b/lib/Makefile index a099885..decc5ee 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -83,6 +83,9 @@ OBJS := $(addprefix $(obj),$(COBJS)) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)bzlib.o: CFLAGS += -mno-unaligned-access + #########################################################################
# defines $(obj).depend target
On Mon, Oct 08, 2012 at 09:50:18PM +0200, Albert ARIBAUD wrote:
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net
We unfortunately have a problem here. This ends up passing -mno-unaligned-access globally, for all platforms. We need something like the following:
Ensure that we only pass -mno-unaligned-access to ARMv7 platforms (where we must ensure this flag is passed so no using call-cc-option).
Signed-off-by; Tom Rini trini@ti.com
diff --git a/arch/arm/cpu/armv7/config.mk b/arch/arm/cpu/armv7/config.mk index 5407cb6..3c5ca23 100644 --- a/arch/arm/cpu/armv7/config.mk +++ b/arch/arm/cpu/armv7/config.mk @@ -34,6 +34,10 @@ PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_ARMV7) # ========================================================================= PF_RELFLAGS_SLB_AT := $(call cc-option,-mshort-load-bytes,$(call cc-option,-malignment-traps,)) PLATFORM_RELFLAGS += $(PF_RELFLAGS_SLB_AT) + +# SEE README.arm-unaligned-accesses +PLATFORM_NO_UNALIGNED := -mno-unaligned-access + ifneq ($(CONFIG_IMX_CONFIG),) ALL-y += $(obj)u-boot.imx endif diff --git a/common/Makefile b/common/Makefile index a498367..33c606a 100644 --- a/common/Makefile +++ b/common/Makefile @@ -233,8 +233,8 @@ $(obj)../tools/envcrc: $(MAKE) -C ../tools
# SEE README.arm-unaligned-accesses -$(obj)hush.o: CFLAGS += -mno-unaligned-access -$(obj)fdt_support.o: CFLAGS += -mno-unaligned-access +$(obj)hush.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) +$(obj)fdt_support.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
#########################################################################
diff --git a/fs/fat/Makefile b/fs/fat/Makefile index 5c4a2aa..02e6881 100644 --- a/fs/fat/Makefile +++ b/fs/fat/Makefile @@ -40,7 +40,7 @@ $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
# SEE README.arm-unaligned-accesses -$(obj)file.o: CFLAGS += -mno-unaligned-access +$(obj)file.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
#########################################################################
diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile index 71c40f2..bfe6874 100644 --- a/fs/ubifs/Makefile +++ b/fs/ubifs/Makefile @@ -43,7 +43,7 @@ $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
# SEE README.arm-unaligned-accesses -$(obj)super.o: CFLAGS += -mno-unaligned-access +$(obj)super.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
#########################################################################
diff --git a/lib/Makefile b/lib/Makefile index decc5ee..e44e045 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -84,7 +84,7 @@ $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
# SEE README.arm-unaligned-accesses -$(obj)bzlib.o: CFLAGS += -mno-unaligned-access +$(obj)bzlib.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
#########################################################################
Hi Tom,
On Tue, 9 Oct 2012 11:34:11 -0700, Tom Rini trini@ti.com wrote:
On Mon, Oct 08, 2012 at 09:50:18PM +0200, Albert ARIBAUD wrote:
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net
We unfortunately have a problem here. This ends up passing -mno-unaligned-access globally, for all platforms. We need something like the following:
General remark: I'd strongly suggest not to copy-paste in a list reply somtheing that looks suspiciously like a (new) patch. It leads to this kind of things: http://patchwork.ozlabs.org/patch/190403/ which may not be what you wanted.
Ensure that we only pass -mno-unaligned-access to ARMv7 platforms (where we must ensure this flag is passed so no using call-cc-option).
Signed-off-by; Tom Rini trini@ti.com
diff --git a/arch/arm/cpu/armv7/config.mk b/arch/arm/cpu/armv7/config.mk index 5407cb6..3c5ca23 100644 --- a/arch/arm/cpu/armv7/config.mk +++ b/arch/arm/cpu/armv7/config.mk @@ -34,6 +34,10 @@ PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_ARMV7) # ========================================================================= PF_RELFLAGS_SLB_AT := $(call cc-option,-mshort-load-bytes,$(call cc-option,-malignment-traps,)) PLATFORM_RELFLAGS += $(PF_RELFLAGS_SLB_AT)
+# SEE README.arm-unaligned-accesses +PLATFORM_NO_UNALIGNED := -mno-unaligned-access
I suspect that you meant
PLATFORM_NO_UNALIGNED := $(call cc-option,-mno-unaligned-access,)
?
ifneq ($(CONFIG_IMX_CONFIG),) ALL-y += $(obj)u-boot.imx endif diff --git a/common/Makefile b/common/Makefile index a498367..33c606a 100644 --- a/common/Makefile +++ b/common/Makefile @@ -233,8 +233,8 @@ $(obj)../tools/envcrc: $(MAKE) -C ../tools
# SEE README.arm-unaligned-accesses -$(obj)hush.o: CFLAGS += -mno-unaligned-access -$(obj)fdt_support.o: CFLAGS += -mno-unaligned-access +$(obj)hush.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) +$(obj)fdt_support.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
#########################################################################
diff --git a/fs/fat/Makefile b/fs/fat/Makefile index 5c4a2aa..02e6881 100644 --- a/fs/fat/Makefile +++ b/fs/fat/Makefile @@ -40,7 +40,7 @@ $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
# SEE README.arm-unaligned-accesses -$(obj)file.o: CFLAGS += -mno-unaligned-access +$(obj)file.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
#########################################################################
diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile index 71c40f2..bfe6874 100644 --- a/fs/ubifs/Makefile +++ b/fs/ubifs/Makefile @@ -43,7 +43,7 @@ $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
# SEE README.arm-unaligned-accesses -$(obj)super.o: CFLAGS += -mno-unaligned-access +$(obj)super.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
#########################################################################
diff --git a/lib/Makefile b/lib/Makefile index decc5ee..e44e045 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -84,7 +84,7 @@ $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
# SEE README.arm-unaligned-accesses -$(obj)bzlib.o: CFLAGS += -mno-unaligned-access +$(obj)bzlib.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
#########################################################################
Will prepare V5 right away.
Amicalement,
On Tue, Oct 09, 2012 at 08:42:26PM +0200, Albert ARIBAUD wrote:
Hi Tom,
On Tue, 9 Oct 2012 11:34:11 -0700, Tom Rini trini@ti.com wrote:
On Mon, Oct 08, 2012 at 09:50:18PM +0200, Albert ARIBAUD wrote:
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net
We unfortunately have a problem here. This ends up passing -mno-unaligned-access globally, for all platforms. We need something like the following:
General remark: I'd strongly suggest not to copy-paste in a list reply somtheing that looks suspiciously like a (new) patch. It leads to this kind of things: http://patchwork.ozlabs.org/patch/190403/ which may not be what you wanted.
Ensure that we only pass -mno-unaligned-access to ARMv7 platforms (where we must ensure this flag is passed so no using call-cc-option).
Signed-off-by; Tom Rini trini@ti.com
diff --git a/arch/arm/cpu/armv7/config.mk b/arch/arm/cpu/armv7/config.mk index 5407cb6..3c5ca23 100644 --- a/arch/arm/cpu/armv7/config.mk +++ b/arch/arm/cpu/armv7/config.mk @@ -34,6 +34,10 @@ PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_ARMV7) # ========================================================================= PF_RELFLAGS_SLB_AT := $(call cc-option,-mshort-load-bytes,$(call cc-option,-malignment-traps,)) PLATFORM_RELFLAGS += $(PF_RELFLAGS_SLB_AT)
+# SEE README.arm-unaligned-accesses +PLATFORM_NO_UNALIGNED := -mno-unaligned-access
I suspect that you meant
PLATFORM_NO_UNALIGNED := $(call cc-option,-mno-unaligned-access,)
This could lead to silently not applying the flag when we want to apply the flag. Just like before, I think we need to force the option, until such time as we can do $(call cc-option,-mdont-unalign-strings-for-fun,-mno-unaligned-access).
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net --- V5: -mno-unaligned-access was applied to all platforms. Make it apply only to armv7. Clarified README.arm-unaligned-accesses re how to fix issue. Included revert of 'release-only' workaround. V4: added information on how to find relocation offset for pc common/Makefile missed a comment re README.arm-unaligned-accesses V3: *really* fix incorrect doc file name in dabort handler message clarifications and typo fixes in README.arm-unaligned-accesses V2: fix incorrect doc file name in dabort handler message
arch/arm/cpu/arm926ejs/orion5x/cpu.c | 4 +- arch/arm/cpu/armv7/config.mk | 6 +- arch/arm/lib/interrupts.c | 2 +- board/ti/omap2420h4/sys_info.c | 28 ++++---- common/Makefile | 4 ++ common/cmd_dfu.c | 2 +- doc/README.arm-unaligned-accesses | 122 ++++++++++++++++++++++++++++++++++ fs/fat/Makefile | 2 + fs/ubifs/Makefile | 3 + lib/Makefile | 3 + 10 files changed, 156 insertions(+), 20 deletions(-) create mode 100644 doc/README.arm-unaligned-accesses
diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c b/arch/arm/cpu/arm926ejs/orion5x/cpu.c index c3948d3..5a4775a 100644 --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c @@ -194,8 +194,8 @@ u32 orion5x_device_rev(void) */ int print_cpuinfo(void) { - char dev_str[] = "0x0000"; - char rev_str[] = "0x00"; + char dev_str[7]; /* room enough for 0x0000 plus null byte */ + char rev_str[5]; /* room enough for 0x00 plus null byte */ char *dev_name = NULL; char *rev_name = NULL;
diff --git a/arch/arm/cpu/armv7/config.mk b/arch/arm/cpu/armv7/config.mk index 560c084..429231e 100644 --- a/arch/arm/cpu/armv7/config.mk +++ b/arch/arm/cpu/armv7/config.mk @@ -26,8 +26,6 @@ PLATFORM_RELFLAGS += -fno-common -ffixed-r8 -msoft-float # supported by more tool-chains PF_CPPFLAGS_ARMV7 := $(call cc-option, -march=armv7-a, -march=armv5) PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_ARMV7) -PF_CPPFLAGS_NO_UNALIGNED := $(call cc-option, -mno-unaligned-access,) -PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_NO_UNALIGNED)
# ========================================================================= # @@ -36,6 +34,10 @@ PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_NO_UNALIGNED) # ========================================================================= PF_RELFLAGS_SLB_AT := $(call cc-option,-mshort-load-bytes,$(call cc-option,-malignment-traps,)) PLATFORM_RELFLAGS += $(PF_RELFLAGS_SLB_AT) + +# SEE README.arm-unaligned-accesses +PLATFORM_NO_UNALIGNED := $(call cc-option, -mno-unaligned-access,) + ifneq ($(CONFIG_IMX_CONFIG),) ALL-y += $(obj)u-boot.imx endif diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c index 74ff5ce..02124a7 100644 --- a/arch/arm/lib/interrupts.c +++ b/arch/arm/lib/interrupts.c @@ -169,7 +169,7 @@ void do_prefetch_abort (struct pt_regs *pt_regs)
void do_data_abort (struct pt_regs *pt_regs) { - printf ("data abort\n"); + printf ("data abort\n\n MAYBE you should read doc/README.arm-unaligned-accesses\n\n"); show_regs (pt_regs); bad_mode (); } diff --git a/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c index a9f7241..b12011e 100644 --- a/board/ti/omap2420h4/sys_info.c +++ b/board/ti/omap2420h4/sys_info.c @@ -237,20 +237,20 @@ u32 wait_on_value(u32 read_bit_mask, u32 match_value, u32 read_addr, u32 bound) *********************************************************************/ void display_board_info(u32 btype) { - char cpu_2420[] = "2420"; /* cpu type */ - char cpu_2422[] = "2422"; - char cpu_2423[] = "2423"; - char db_men[] = "Menelaus"; /* board type */ - char db_ip[] = "IP"; - char mem_sdr[] = "mSDR"; /* memory type */ - char mem_ddr[] = "mDDR"; - char t_tst[] = "TST"; /* security level */ - char t_emu[] = "EMU"; - char t_hs[] = "HS"; - char t_gp[] = "GP"; - char unk[] = "?"; - - char *cpu_s, *db_s, *mem_s, *sec_s; + static const char cpu_2420 [] = "2420"; /* cpu type */ + static const char cpu_2422 [] = "2422"; + static const char cpu_2423 [] = "2423"; + static const char db_men [] = "Menelaus"; /* board type */ + static const char db_ip [] = "IP"; + static const char mem_sdr [] = "mSDR"; /* memory type */ + static const char mem_ddr [] = "mDDR"; + static const char t_tst [] = "TST"; /* security level */ + static const char t_emu [] = "EMU"; + static const char t_hs [] = "HS"; + static const char t_gp [] = "GP"; + static const char unk [] = "?"; + + const char *cpu_s, *db_s, *mem_s, *sec_s; u32 cpu, rev, sec;
rev = get_cpu_rev(); diff --git a/common/Makefile b/common/Makefile index 5442fbb..8a85dec 100644 --- a/common/Makefile +++ b/common/Makefile @@ -231,6 +231,10 @@ $(obj)env_embedded.o: $(src)env_embedded.c $(obj)../tools/envcrc $(obj)../tools/envcrc: $(MAKE) -C ../tools
+# SEE README.arm-unaligned-accesses +$(obj)hush.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) +$(obj)fdt_support.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) + #########################################################################
# defines $(obj).depend target diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index 62fb890..01d6b3a 100644 --- a/common/cmd_dfu.c +++ b/common/cmd_dfu.c @@ -30,7 +30,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { const char *str_env; - char s[] = "dfu"; + char *s = "dfu"; char *env_bkp; int ret;
diff --git a/doc/README.arm-unaligned-accesses b/doc/README.arm-unaligned-accesses new file mode 100644 index 0000000..c37d135 --- /dev/null +++ b/doc/README.arm-unaligned-accesses @@ -0,0 +1,122 @@ +If you are reading this because of a data abort: the following MIGHT +be relevant to your abort, if it was caused by an alignment violation. +In order to determine this, use the PC from the abort dump along with +an objdump -s -S of the u-boot ELF binary to locate the function where +the abort happened; then compare this function with the examples below. +If they match, then you've been hit with a compiler generated unaligned +access, and you should rewrite your code or add -mno-unaligned-access +to the command line of the offending file. + +Note that the PC shown in the abort message is relocated. In order to +be able to match it to an address in the ELF binary dump, you will need +to know the relocation offset. If your target defines CONFIG_CMD_BDI +and if you can get to the prompt and enter commands before the abort +happens, then command "bdinfo" will give you the offset. Otherwise you +will need to try a build with DEBUG set, which will display the offset, +or use a debugger and set a breakpoint at relocate_code() to see the +offset (passed as an argument). + +* + +Since U-Boot runs on a variety of hardware, some only able to perform +unaligned accesses with a strong penalty, some unable to perform them +at all, the policy regarding unaligned accesses is to not perform any, +unless absolutely necessary because of hardware or standards. + +Also, on hardware which permits it, the core is configured to throw +data abort exceptions on unaligned accesses in order to catch these +unallowed accesses as early as possible. + +Until version 4.7, the gcc default for performing unaligned accesses +(-mno-unaligned-access) is to emulate unaligned accesses using aligned +loads and stores plus shifts and masks. Emulated unaligned accesses +will not be caught by hardware. These accesses may be costly and may +be actually unnecessary. In order to catch these accesses and remove +or optimize them, option -munaligned-access is explicitly set for all +versions of gcc which support it. + +From gcc 4.7 onward starting at armv7 architectures, the default for +performing unaligned accesses is to use unaligned native loads and +stores (-munaligned-access), because the cost of unaligned accesses +has dropped on armv7 and beyond. This should not affect U-Boot's +policy of controlling unaligned accesses, however the compiler may +generate uncontrolled unaligned accesses on its own in at least one +known case: when declaring a local initialized char array, e.g. + +function foo() +{ + char buffer[] = "initial value"; +/* or */ + char buffer[] = { 'i', 'n', 'i', 't', 0 }; + ... +} + +Under -munaligned-accesses with optimizations on, this declaration +causes the compiler to generate native loads from the literal string +and native stores to the buffer, and the literal string alignment +cannot be controlled. If it is misaligned, then the core will throw +a data abort exception. + +Quite probably the same might happen for 16-bit array initializations +where the constant is aligned on a boundary which is a multiple of 2 +but not of 4: + +function foo() +{ + u16 buffer[] = { 1, 2, 3 }; + ... +} + +The long term solution to this issue is to add an option to gcc to +allow controlling the general alignment of data, including constant +initialization values. + +However this will only apply to the version of gcc which will have such +an option. For other versions, there are four workarounds: + +a) Enforce as a rule that array initializations as described above + are forbidden. This is generally not acceptable as they are valid, + and usual, C constructs. The only case where they could be rejected + is when they actually equate to a const char* declaration, i.e. the + array is initialized and never modified in the function's scope. + +b) Drop the requirement on unaligned accesses at least for ARMv7, + i.e. do not throw a data abort exception upon unaligned accesses. + But that will allow adding badly aligned code to U-Boot, only for + it to fail when re-used with a stricter target, possibly once the + bad code is already in mainline. + +c) Relax the -munaligned-access rule globally. This will prevent native + unaligned accesses of course, but that will also hide any bug caused + by a bad unaligned access, making it much harder to diagnose it. It + is actually what already happens when building ARM targets with a + pre-4.7 gcc, and it may actually already hide some bugs yet unseen + until the target gets compiled with -munaligned-access. + +d) Relax the -munaligned-access rule only for for files susceptible to + the local initialized array issue and for armv7 architectures and + beyond. This minimizes the quantity of code which can hide unwanted + misaligned accesses. + +The option retained is d). + +Considering that actual occurrences of the issue are rare (as of this +writing, 5 files out of 7840 in U-Boot, or .3%, contain an initialized +local char array which cannot actually be replaced with a const char*), +contributors should not be required to systematically try and detect +the issue in their patches. + +Detecting files susceptible to the issue can be automated through a +filter installed as a hook in .git which recognizes local char array +initializations. Automation should err on the false positive side, for +instance flagging non-local arrays as if they were local if they cannot +be told apart. + +In any case, detection shall not prevent committing the patch, but +shall pre-populate the commit message with a note to the effect that +this patch contains an initialized local char or 16-bit array and thus +should be protected from the gcc 4.7 issue. + +Upon a positive detection, either $(PLATFORM_NO_UNALIGNED) should be +added to CFLAGS for the affected file(s), or if the array is a pseudo +const char*, it should be replaced by an actual one. diff --git a/fs/fat/Makefile b/fs/fat/Makefile index 9635d36..02e6881 100644 --- a/fs/fat/Makefile +++ b/fs/fat/Makefile @@ -39,6 +39,8 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)file.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
#########################################################################
diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile index ccffe85..bfe6874 100644 --- a/fs/ubifs/Makefile +++ b/fs/ubifs/Makefile @@ -42,6 +42,9 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)super.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) + #########################################################################
# defines $(obj).depend target diff --git a/lib/Makefile b/lib/Makefile index a099885..e44e045 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -83,6 +83,9 @@ OBJS := $(addprefix $(obj),$(COBJS)) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)bzlib.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) + #########################################################################
# defines $(obj).depend target
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net --- V6: Make sure that gcc does not silently drop -mno-unaligned-access if it does not support it. V5: -mno-unaligned-access was applied to all platforms. Make it apply only to armv7. Clarified README.arm-unaligned-accesses re how to fix issue. Included revert of 'release-only' workaround. V4: added information on how to find relocation offset for pc common/Makefile missed a comment re README.arm-unaligned-accesses V3: *really* fix incorrect doc file name in dabort handler message clarifications and typo fixes in README.arm-unaligned-accesses V2: fix incorrect doc file name in dabort handler message
arch/arm/cpu/arm926ejs/orion5x/cpu.c | 4 +- arch/arm/cpu/armv7/config.mk | 6 +- arch/arm/lib/interrupts.c | 2 +- board/ti/omap2420h4/sys_info.c | 28 ++++---- common/Makefile | 4 ++ common/cmd_dfu.c | 2 +- doc/README.arm-unaligned-accesses | 122 ++++++++++++++++++++++++++++++++++ fs/fat/Makefile | 2 + fs/ubifs/Makefile | 3 + lib/Makefile | 3 + 10 files changed, 156 insertions(+), 20 deletions(-) create mode 100644 doc/README.arm-unaligned-accesses
diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c b/arch/arm/cpu/arm926ejs/orion5x/cpu.c index c3948d3..5a4775a 100644 --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c @@ -194,8 +194,8 @@ u32 orion5x_device_rev(void) */ int print_cpuinfo(void) { - char dev_str[] = "0x0000"; - char rev_str[] = "0x00"; + char dev_str[7]; /* room enough for 0x0000 plus null byte */ + char rev_str[5]; /* room enough for 0x00 plus null byte */ char *dev_name = NULL; char *rev_name = NULL;
diff --git a/arch/arm/cpu/armv7/config.mk b/arch/arm/cpu/armv7/config.mk index 560c084..3c5ca23 100644 --- a/arch/arm/cpu/armv7/config.mk +++ b/arch/arm/cpu/armv7/config.mk @@ -26,8 +26,6 @@ PLATFORM_RELFLAGS += -fno-common -ffixed-r8 -msoft-float # supported by more tool-chains PF_CPPFLAGS_ARMV7 := $(call cc-option, -march=armv7-a, -march=armv5) PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_ARMV7) -PF_CPPFLAGS_NO_UNALIGNED := $(call cc-option, -mno-unaligned-access,) -PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_NO_UNALIGNED)
# ========================================================================= # @@ -36,6 +34,10 @@ PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_NO_UNALIGNED) # ========================================================================= PF_RELFLAGS_SLB_AT := $(call cc-option,-mshort-load-bytes,$(call cc-option,-malignment-traps,)) PLATFORM_RELFLAGS += $(PF_RELFLAGS_SLB_AT) + +# SEE README.arm-unaligned-accesses +PLATFORM_NO_UNALIGNED := -mno-unaligned-access + ifneq ($(CONFIG_IMX_CONFIG),) ALL-y += $(obj)u-boot.imx endif diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c index 74ff5ce..02124a7 100644 --- a/arch/arm/lib/interrupts.c +++ b/arch/arm/lib/interrupts.c @@ -169,7 +169,7 @@ void do_prefetch_abort (struct pt_regs *pt_regs)
void do_data_abort (struct pt_regs *pt_regs) { - printf ("data abort\n"); + printf ("data abort\n\n MAYBE you should read doc/README.arm-unaligned-accesses\n\n"); show_regs (pt_regs); bad_mode (); } diff --git a/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c index a9f7241..b12011e 100644 --- a/board/ti/omap2420h4/sys_info.c +++ b/board/ti/omap2420h4/sys_info.c @@ -237,20 +237,20 @@ u32 wait_on_value(u32 read_bit_mask, u32 match_value, u32 read_addr, u32 bound) *********************************************************************/ void display_board_info(u32 btype) { - char cpu_2420[] = "2420"; /* cpu type */ - char cpu_2422[] = "2422"; - char cpu_2423[] = "2423"; - char db_men[] = "Menelaus"; /* board type */ - char db_ip[] = "IP"; - char mem_sdr[] = "mSDR"; /* memory type */ - char mem_ddr[] = "mDDR"; - char t_tst[] = "TST"; /* security level */ - char t_emu[] = "EMU"; - char t_hs[] = "HS"; - char t_gp[] = "GP"; - char unk[] = "?"; - - char *cpu_s, *db_s, *mem_s, *sec_s; + static const char cpu_2420 [] = "2420"; /* cpu type */ + static const char cpu_2422 [] = "2422"; + static const char cpu_2423 [] = "2423"; + static const char db_men [] = "Menelaus"; /* board type */ + static const char db_ip [] = "IP"; + static const char mem_sdr [] = "mSDR"; /* memory type */ + static const char mem_ddr [] = "mDDR"; + static const char t_tst [] = "TST"; /* security level */ + static const char t_emu [] = "EMU"; + static const char t_hs [] = "HS"; + static const char t_gp [] = "GP"; + static const char unk [] = "?"; + + const char *cpu_s, *db_s, *mem_s, *sec_s; u32 cpu, rev, sec;
rev = get_cpu_rev(); diff --git a/common/Makefile b/common/Makefile index 5442fbb..8a85dec 100644 --- a/common/Makefile +++ b/common/Makefile @@ -231,6 +231,10 @@ $(obj)env_embedded.o: $(src)env_embedded.c $(obj)../tools/envcrc $(obj)../tools/envcrc: $(MAKE) -C ../tools
+# SEE README.arm-unaligned-accesses +$(obj)hush.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) +$(obj)fdt_support.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) + #########################################################################
# defines $(obj).depend target diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index 62fb890..01d6b3a 100644 --- a/common/cmd_dfu.c +++ b/common/cmd_dfu.c @@ -30,7 +30,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { const char *str_env; - char s[] = "dfu"; + char *s = "dfu"; char *env_bkp; int ret;
diff --git a/doc/README.arm-unaligned-accesses b/doc/README.arm-unaligned-accesses new file mode 100644 index 0000000..c37d135 --- /dev/null +++ b/doc/README.arm-unaligned-accesses @@ -0,0 +1,122 @@ +If you are reading this because of a data abort: the following MIGHT +be relevant to your abort, if it was caused by an alignment violation. +In order to determine this, use the PC from the abort dump along with +an objdump -s -S of the u-boot ELF binary to locate the function where +the abort happened; then compare this function with the examples below. +If they match, then you've been hit with a compiler generated unaligned +access, and you should rewrite your code or add -mno-unaligned-access +to the command line of the offending file. + +Note that the PC shown in the abort message is relocated. In order to +be able to match it to an address in the ELF binary dump, you will need +to know the relocation offset. If your target defines CONFIG_CMD_BDI +and if you can get to the prompt and enter commands before the abort +happens, then command "bdinfo" will give you the offset. Otherwise you +will need to try a build with DEBUG set, which will display the offset, +or use a debugger and set a breakpoint at relocate_code() to see the +offset (passed as an argument). + +* + +Since U-Boot runs on a variety of hardware, some only able to perform +unaligned accesses with a strong penalty, some unable to perform them +at all, the policy regarding unaligned accesses is to not perform any, +unless absolutely necessary because of hardware or standards. + +Also, on hardware which permits it, the core is configured to throw +data abort exceptions on unaligned accesses in order to catch these +unallowed accesses as early as possible. + +Until version 4.7, the gcc default for performing unaligned accesses +(-mno-unaligned-access) is to emulate unaligned accesses using aligned +loads and stores plus shifts and masks. Emulated unaligned accesses +will not be caught by hardware. These accesses may be costly and may +be actually unnecessary. In order to catch these accesses and remove +or optimize them, option -munaligned-access is explicitly set for all +versions of gcc which support it. + +From gcc 4.7 onward starting at armv7 architectures, the default for +performing unaligned accesses is to use unaligned native loads and +stores (-munaligned-access), because the cost of unaligned accesses +has dropped on armv7 and beyond. This should not affect U-Boot's +policy of controlling unaligned accesses, however the compiler may +generate uncontrolled unaligned accesses on its own in at least one +known case: when declaring a local initialized char array, e.g. + +function foo() +{ + char buffer[] = "initial value"; +/* or */ + char buffer[] = { 'i', 'n', 'i', 't', 0 }; + ... +} + +Under -munaligned-accesses with optimizations on, this declaration +causes the compiler to generate native loads from the literal string +and native stores to the buffer, and the literal string alignment +cannot be controlled. If it is misaligned, then the core will throw +a data abort exception. + +Quite probably the same might happen for 16-bit array initializations +where the constant is aligned on a boundary which is a multiple of 2 +but not of 4: + +function foo() +{ + u16 buffer[] = { 1, 2, 3 }; + ... +} + +The long term solution to this issue is to add an option to gcc to +allow controlling the general alignment of data, including constant +initialization values. + +However this will only apply to the version of gcc which will have such +an option. For other versions, there are four workarounds: + +a) Enforce as a rule that array initializations as described above + are forbidden. This is generally not acceptable as they are valid, + and usual, C constructs. The only case where they could be rejected + is when they actually equate to a const char* declaration, i.e. the + array is initialized and never modified in the function's scope. + +b) Drop the requirement on unaligned accesses at least for ARMv7, + i.e. do not throw a data abort exception upon unaligned accesses. + But that will allow adding badly aligned code to U-Boot, only for + it to fail when re-used with a stricter target, possibly once the + bad code is already in mainline. + +c) Relax the -munaligned-access rule globally. This will prevent native + unaligned accesses of course, but that will also hide any bug caused + by a bad unaligned access, making it much harder to diagnose it. It + is actually what already happens when building ARM targets with a + pre-4.7 gcc, and it may actually already hide some bugs yet unseen + until the target gets compiled with -munaligned-access. + +d) Relax the -munaligned-access rule only for for files susceptible to + the local initialized array issue and for armv7 architectures and + beyond. This minimizes the quantity of code which can hide unwanted + misaligned accesses. + +The option retained is d). + +Considering that actual occurrences of the issue are rare (as of this +writing, 5 files out of 7840 in U-Boot, or .3%, contain an initialized +local char array which cannot actually be replaced with a const char*), +contributors should not be required to systematically try and detect +the issue in their patches. + +Detecting files susceptible to the issue can be automated through a +filter installed as a hook in .git which recognizes local char array +initializations. Automation should err on the false positive side, for +instance flagging non-local arrays as if they were local if they cannot +be told apart. + +In any case, detection shall not prevent committing the patch, but +shall pre-populate the commit message with a note to the effect that +this patch contains an initialized local char or 16-bit array and thus +should be protected from the gcc 4.7 issue. + +Upon a positive detection, either $(PLATFORM_NO_UNALIGNED) should be +added to CFLAGS for the affected file(s), or if the array is a pseudo +const char*, it should be replaced by an actual one. diff --git a/fs/fat/Makefile b/fs/fat/Makefile index 9635d36..02e6881 100644 --- a/fs/fat/Makefile +++ b/fs/fat/Makefile @@ -39,6 +39,8 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)file.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
#########################################################################
diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile index ccffe85..bfe6874 100644 --- a/fs/ubifs/Makefile +++ b/fs/ubifs/Makefile @@ -42,6 +42,9 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)super.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) + #########################################################################
# defines $(obj).depend target diff --git a/lib/Makefile b/lib/Makefile index a099885..e44e045 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -83,6 +83,9 @@ OBJS := $(addprefix $(obj),$(COBJS)) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)bzlib.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) + #########################################################################
# defines $(obj).depend target
This patch has been sent without a proper Vx indication.
Resending.
Amicalement,
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net --- V6: Make sure that gcc does not silently drop -mno-unaligned-access if it does not support it. V5: -mno-unaligned-access was applied to all platforms. Make it apply only to armv7. Clarified README.arm-unaligned-accesses re how to fix issue. Included revert of 'release-only' workaround. V4: added information on how to find relocation offset for pc common/Makefile missed a comment re README.arm-unaligned-accesses V3: *really* fix incorrect doc file name in dabort handler message clarifications and typo fixes in README.arm-unaligned-accesses V2: fix incorrect doc file name in dabort handler message
arch/arm/cpu/arm926ejs/orion5x/cpu.c | 4 +- arch/arm/cpu/armv7/config.mk | 6 +- arch/arm/lib/interrupts.c | 2 +- board/ti/omap2420h4/sys_info.c | 28 ++++---- common/Makefile | 4 ++ common/cmd_dfu.c | 2 +- doc/README.arm-unaligned-accesses | 122 ++++++++++++++++++++++++++++++++++ fs/fat/Makefile | 2 + fs/ubifs/Makefile | 3 + lib/Makefile | 3 + 10 files changed, 156 insertions(+), 20 deletions(-) create mode 100644 doc/README.arm-unaligned-accesses
diff --git a/arch/arm/cpu/arm926ejs/orion5x/cpu.c b/arch/arm/cpu/arm926ejs/orion5x/cpu.c index c3948d3..5a4775a 100644 --- a/arch/arm/cpu/arm926ejs/orion5x/cpu.c +++ b/arch/arm/cpu/arm926ejs/orion5x/cpu.c @@ -194,8 +194,8 @@ u32 orion5x_device_rev(void) */ int print_cpuinfo(void) { - char dev_str[] = "0x0000"; - char rev_str[] = "0x00"; + char dev_str[7]; /* room enough for 0x0000 plus null byte */ + char rev_str[5]; /* room enough for 0x00 plus null byte */ char *dev_name = NULL; char *rev_name = NULL;
diff --git a/arch/arm/cpu/armv7/config.mk b/arch/arm/cpu/armv7/config.mk index 560c084..3c5ca23 100644 --- a/arch/arm/cpu/armv7/config.mk +++ b/arch/arm/cpu/armv7/config.mk @@ -26,8 +26,6 @@ PLATFORM_RELFLAGS += -fno-common -ffixed-r8 -msoft-float # supported by more tool-chains PF_CPPFLAGS_ARMV7 := $(call cc-option, -march=armv7-a, -march=armv5) PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_ARMV7) -PF_CPPFLAGS_NO_UNALIGNED := $(call cc-option, -mno-unaligned-access,) -PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_NO_UNALIGNED)
# ========================================================================= # @@ -36,6 +34,10 @@ PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_NO_UNALIGNED) # ========================================================================= PF_RELFLAGS_SLB_AT := $(call cc-option,-mshort-load-bytes,$(call cc-option,-malignment-traps,)) PLATFORM_RELFLAGS += $(PF_RELFLAGS_SLB_AT) + +# SEE README.arm-unaligned-accesses +PLATFORM_NO_UNALIGNED := -mno-unaligned-access + ifneq ($(CONFIG_IMX_CONFIG),) ALL-y += $(obj)u-boot.imx endif diff --git a/arch/arm/lib/interrupts.c b/arch/arm/lib/interrupts.c index 74ff5ce..02124a7 100644 --- a/arch/arm/lib/interrupts.c +++ b/arch/arm/lib/interrupts.c @@ -169,7 +169,7 @@ void do_prefetch_abort (struct pt_regs *pt_regs)
void do_data_abort (struct pt_regs *pt_regs) { - printf ("data abort\n"); + printf ("data abort\n\n MAYBE you should read doc/README.arm-unaligned-accesses\n\n"); show_regs (pt_regs); bad_mode (); } diff --git a/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c index a9f7241..b12011e 100644 --- a/board/ti/omap2420h4/sys_info.c +++ b/board/ti/omap2420h4/sys_info.c @@ -237,20 +237,20 @@ u32 wait_on_value(u32 read_bit_mask, u32 match_value, u32 read_addr, u32 bound) *********************************************************************/ void display_board_info(u32 btype) { - char cpu_2420[] = "2420"; /* cpu type */ - char cpu_2422[] = "2422"; - char cpu_2423[] = "2423"; - char db_men[] = "Menelaus"; /* board type */ - char db_ip[] = "IP"; - char mem_sdr[] = "mSDR"; /* memory type */ - char mem_ddr[] = "mDDR"; - char t_tst[] = "TST"; /* security level */ - char t_emu[] = "EMU"; - char t_hs[] = "HS"; - char t_gp[] = "GP"; - char unk[] = "?"; - - char *cpu_s, *db_s, *mem_s, *sec_s; + static const char cpu_2420 [] = "2420"; /* cpu type */ + static const char cpu_2422 [] = "2422"; + static const char cpu_2423 [] = "2423"; + static const char db_men [] = "Menelaus"; /* board type */ + static const char db_ip [] = "IP"; + static const char mem_sdr [] = "mSDR"; /* memory type */ + static const char mem_ddr [] = "mDDR"; + static const char t_tst [] = "TST"; /* security level */ + static const char t_emu [] = "EMU"; + static const char t_hs [] = "HS"; + static const char t_gp [] = "GP"; + static const char unk [] = "?"; + + const char *cpu_s, *db_s, *mem_s, *sec_s; u32 cpu, rev, sec;
rev = get_cpu_rev(); diff --git a/common/Makefile b/common/Makefile index 5442fbb..8a85dec 100644 --- a/common/Makefile +++ b/common/Makefile @@ -231,6 +231,10 @@ $(obj)env_embedded.o: $(src)env_embedded.c $(obj)../tools/envcrc $(obj)../tools/envcrc: $(MAKE) -C ../tools
+# SEE README.arm-unaligned-accesses +$(obj)hush.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) +$(obj)fdt_support.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) + #########################################################################
# defines $(obj).depend target diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c index 62fb890..01d6b3a 100644 --- a/common/cmd_dfu.c +++ b/common/cmd_dfu.c @@ -30,7 +30,7 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { const char *str_env; - char s[] = "dfu"; + char *s = "dfu"; char *env_bkp; int ret;
diff --git a/doc/README.arm-unaligned-accesses b/doc/README.arm-unaligned-accesses new file mode 100644 index 0000000..c37d135 --- /dev/null +++ b/doc/README.arm-unaligned-accesses @@ -0,0 +1,122 @@ +If you are reading this because of a data abort: the following MIGHT +be relevant to your abort, if it was caused by an alignment violation. +In order to determine this, use the PC from the abort dump along with +an objdump -s -S of the u-boot ELF binary to locate the function where +the abort happened; then compare this function with the examples below. +If they match, then you've been hit with a compiler generated unaligned +access, and you should rewrite your code or add -mno-unaligned-access +to the command line of the offending file. + +Note that the PC shown in the abort message is relocated. In order to +be able to match it to an address in the ELF binary dump, you will need +to know the relocation offset. If your target defines CONFIG_CMD_BDI +and if you can get to the prompt and enter commands before the abort +happens, then command "bdinfo" will give you the offset. Otherwise you +will need to try a build with DEBUG set, which will display the offset, +or use a debugger and set a breakpoint at relocate_code() to see the +offset (passed as an argument). + +* + +Since U-Boot runs on a variety of hardware, some only able to perform +unaligned accesses with a strong penalty, some unable to perform them +at all, the policy regarding unaligned accesses is to not perform any, +unless absolutely necessary because of hardware or standards. + +Also, on hardware which permits it, the core is configured to throw +data abort exceptions on unaligned accesses in order to catch these +unallowed accesses as early as possible. + +Until version 4.7, the gcc default for performing unaligned accesses +(-mno-unaligned-access) is to emulate unaligned accesses using aligned +loads and stores plus shifts and masks. Emulated unaligned accesses +will not be caught by hardware. These accesses may be costly and may +be actually unnecessary. In order to catch these accesses and remove +or optimize them, option -munaligned-access is explicitly set for all +versions of gcc which support it. + +From gcc 4.7 onward starting at armv7 architectures, the default for +performing unaligned accesses is to use unaligned native loads and +stores (-munaligned-access), because the cost of unaligned accesses +has dropped on armv7 and beyond. This should not affect U-Boot's +policy of controlling unaligned accesses, however the compiler may +generate uncontrolled unaligned accesses on its own in at least one +known case: when declaring a local initialized char array, e.g. + +function foo() +{ + char buffer[] = "initial value"; +/* or */ + char buffer[] = { 'i', 'n', 'i', 't', 0 }; + ... +} + +Under -munaligned-accesses with optimizations on, this declaration +causes the compiler to generate native loads from the literal string +and native stores to the buffer, and the literal string alignment +cannot be controlled. If it is misaligned, then the core will throw +a data abort exception. + +Quite probably the same might happen for 16-bit array initializations +where the constant is aligned on a boundary which is a multiple of 2 +but not of 4: + +function foo() +{ + u16 buffer[] = { 1, 2, 3 }; + ... +} + +The long term solution to this issue is to add an option to gcc to +allow controlling the general alignment of data, including constant +initialization values. + +However this will only apply to the version of gcc which will have such +an option. For other versions, there are four workarounds: + +a) Enforce as a rule that array initializations as described above + are forbidden. This is generally not acceptable as they are valid, + and usual, C constructs. The only case where they could be rejected + is when they actually equate to a const char* declaration, i.e. the + array is initialized and never modified in the function's scope. + +b) Drop the requirement on unaligned accesses at least for ARMv7, + i.e. do not throw a data abort exception upon unaligned accesses. + But that will allow adding badly aligned code to U-Boot, only for + it to fail when re-used with a stricter target, possibly once the + bad code is already in mainline. + +c) Relax the -munaligned-access rule globally. This will prevent native + unaligned accesses of course, but that will also hide any bug caused + by a bad unaligned access, making it much harder to diagnose it. It + is actually what already happens when building ARM targets with a + pre-4.7 gcc, and it may actually already hide some bugs yet unseen + until the target gets compiled with -munaligned-access. + +d) Relax the -munaligned-access rule only for for files susceptible to + the local initialized array issue and for armv7 architectures and + beyond. This minimizes the quantity of code which can hide unwanted + misaligned accesses. + +The option retained is d). + +Considering that actual occurrences of the issue are rare (as of this +writing, 5 files out of 7840 in U-Boot, or .3%, contain an initialized +local char array which cannot actually be replaced with a const char*), +contributors should not be required to systematically try and detect +the issue in their patches. + +Detecting files susceptible to the issue can be automated through a +filter installed as a hook in .git which recognizes local char array +initializations. Automation should err on the false positive side, for +instance flagging non-local arrays as if they were local if they cannot +be told apart. + +In any case, detection shall not prevent committing the patch, but +shall pre-populate the commit message with a note to the effect that +this patch contains an initialized local char or 16-bit array and thus +should be protected from the gcc 4.7 issue. + +Upon a positive detection, either $(PLATFORM_NO_UNALIGNED) should be +added to CFLAGS for the affected file(s), or if the array is a pseudo +const char*, it should be replaced by an actual one. diff --git a/fs/fat/Makefile b/fs/fat/Makefile index 9635d36..02e6881 100644 --- a/fs/fat/Makefile +++ b/fs/fat/Makefile @@ -39,6 +39,8 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)file.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
#########################################################################
diff --git a/fs/ubifs/Makefile b/fs/ubifs/Makefile index ccffe85..bfe6874 100644 --- a/fs/ubifs/Makefile +++ b/fs/ubifs/Makefile @@ -42,6 +42,9 @@ all: $(LIB) $(AOBJS) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)super.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) + #########################################################################
# defines $(obj).depend target diff --git a/lib/Makefile b/lib/Makefile index a099885..e44e045 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -83,6 +83,9 @@ OBJS := $(addprefix $(obj),$(COBJS)) $(LIB): $(obj).depend $(OBJS) $(call cmd_link_o_target, $(OBJS))
+# SEE README.arm-unaligned-accesses +$(obj)bzlib.o: CFLAGS += $(PLATFORM_NO_UNALIGNED) + #########################################################################
# defines $(obj).depend target
On Tue, 9 Oct 2012 21:28:15 +0200, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net
Tom,
As per our discussion, this patch should be placed in u-boot/next right above the 2012-10 release commit.
At this time, just after a git fech u-boot, I don't see the patch in next at all.
Do you want me to prepare an ad hoc branch with this patch then the current content of u-boot/next, above v2012.10-rc3, so that you just have to git reset --hard your next on it?
Amicalement,
On Sun, Oct 14, 2012 at 1:48 AM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
On Tue, 9 Oct 2012 21:28:15 +0200, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net
Tom,
As per our discussion, this patch should be placed in u-boot/next right above the 2012-10 release commit.
At this time, just after a git fech u-boot, I don't see the patch in next at all.
Do you want me to prepare an ad hoc branch with this patch then the current content of u-boot/next, above v2012.10-rc3, so that you just have to git reset --hard your next on it?
Amicalement,
As the questions about our usage of rebasing in 'next' have shown, it's not quite as cut and dry as I had hoped to rebase next at my whim (and long term, that's a good thing I think). My plan tomorrow (or today, depending on your local timezone) is to release v2012.10, apply this v6 patch and then bring in next to master, see that everything is still building and go from there.
Hi Tom,
On Sun, 14 Oct 2012 22:05:15 -0700, Tom Rini trini@ti.com wrote:
On Sun, Oct 14, 2012 at 1:48 AM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
On Tue, 9 Oct 2012 21:28:15 +0200, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net
Tom,
As per our discussion, this patch should be placed in u-boot/next right above the 2012-10 release commit.
At this time, just after a git fech u-boot, I don't see the patch in next at all.
Do you want me to prepare an ad hoc branch with this patch then the current content of u-boot/next, above v2012.10-rc3, so that you just have to git reset --hard your next on it?
Amicalement,
As the questions about our usage of rebasing in 'next' have shown, it's not quite as cut and dry as I had hoped to rebase next at my whim (and long term, that's a good thing I think). My plan tomorrow (or today, depending on your local timezone) is to release v2012.10, apply this v6 patch and then bring in next to master, see that everything is still building and go from there.
Fine with me, thanks!
P.S. Until this release, I'd worked under the assumption that next could be more freely rebased than master is. From now on, I will follow the same principle for next as for master, i.e. not rebase but merge (possibly ff) from other and only rollback if required and appropriate.
Amicalement,
On Tue, Oct 09, 2012 at 09:28:15PM +0200, Albert ARIBAUD wrote:
Under option -munaligned-access, gcc can perform local char or 16-bit array initializations using misaligned native accesses which will throw a data abort exception. Fix files where these array initializations were unneeded, and for files known to contain such initializations, enforce gcc option -mno-unaligned-access.
Signed-off-by: Albert ARIBAUD albert.u.boot@aribaud.net
V6: Make sure that gcc does not silently drop -mno-unaligned-access if it does not support it.
OK, you were correct. We need to use cc-option since older compilers which were not causing issues don't support it either, making this workable. I will just fix this in-line rather than make you do a v7.
participants (3)
-
Albert ARIBAUD -
Andreas Bießmann -
Tom Rini