[U-Boot] [PATCH] ARM: prevent misaligned array inits

* [U-Boot] [PATCH] ARM: prevent misaligned array inits
@ 2012-10-04 21:49 Albert ARIBAUD
  2012-10-05 19:15 ` [U-Boot] [PATCH V2] " Albert ARIBAUD
  0 siblings, 1 reply; 20+ messages in thread
From: Albert ARIBAUD @ 2012-10-04 21:49 UTC (permalink / raw)
  To: u-boot

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
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 20+ messages in thread