public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
* [U-Boot] [RFC] ARM: prevent misaligned array inits
@ 2012-10-02 18:46 Albert ARIBAUD
  2012-10-02 19:13 ` Joakim Tjernlund
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Albert ARIBAUD @ 2012-10-02 18:46 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>
---
Please test this patch with gcc 4.7 on boards which do data aborts
or resets due to misaligned accesses and report result to me.

 arch/arm/cpu/arm926ejs/orion5x/cpu.c |    4 +-
 board/ti/omap2420h4/sys_info.c       |   24 ++++-----
 common/Makefile                      |    3 ++
 common/cmd_dfu.c                     |    2 +-
 doc/README.arm-unaligned-accesses    |   95 ++++++++++++++++++++++++++++++++++
 fs/fat/Makefile                      |    2 +
 fs/ubifs/Makefile                    |    3 ++
 lib/Makefile                         |    3 ++
 8 files changed, 121 insertions(+), 15 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/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c
index a9f7241..b462aa5 100644
--- a/board/ti/omap2420h4/sys_info.c
+++ b/board/ti/omap2420h4/sys_info.c
@@ -237,18 +237,18 @@ 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_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;
 	u32 cpu, rev, sec;
diff --git a/common/Makefile b/common/Makefile
index 125b2be..19b2130 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -227,6 +227,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..00fb1c0
--- /dev/null
+++ b/doc/README.arm-unaligned-accesses
@@ -0,0 +1,95 @@
+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 -munified-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 m-unaligned-access.
+
+d) Relax the -munified-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.
+
+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.
+
+Luckily, detecting files susceptible to the issue can be automated
+through a filter/regexp/script which would recognize local char array
+initializations. Automated 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 be informative only and shall not prevent
+applying the patch.
+
+Upon a positive detection, either option -mno-unaligned-access is
+applied (if not already) to the affected file(s), or if the array is a
+hidden 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 45798de..44393ed 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -78,6 +78,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] 11+ messages in thread

* [U-Boot] [RFC] ARM: prevent misaligned array inits
  2012-10-02 18:46 [U-Boot] [RFC] ARM: prevent misaligned array inits Albert ARIBAUD
@ 2012-10-02 19:13 ` Joakim Tjernlund
  2012-10-02 20:05   ` Albert ARIBAUD
  2012-10-02 20:06 ` Stephen Warren
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Joakim Tjernlund @ 2012-10-02 19:13 UTC (permalink / raw)
  To: u-boot



>   *********************************************************************/
>  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_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 = "?";

hmm, on ppc I think this will cause relocation entries which will build size.

 Jocke

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

* [U-Boot] [RFC] ARM: prevent misaligned array inits
  2012-10-02 19:13 ` Joakim Tjernlund
@ 2012-10-02 20:05   ` Albert ARIBAUD
  2012-10-03  8:22     ` Joakim Tjernlund
  0 siblings, 1 reply; 11+ messages in thread
From: Albert ARIBAUD @ 2012-10-02 20:05 UTC (permalink / raw)
  To: u-boot

Hi Joakim,

On Tue, 2 Oct 2012 21:13:39 +0200, Joakim Tjernlund
<joakim.tjernlund@transmode.se> wrote:

> 
> 
> >   *********************************************************************/
> >  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_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 = "?";
> 
> hmm, on ppc I think this will cause relocation entries which will build size.
> 
>  Jocke

Can you try it and post results with and without it? Sizes and if
possible disassembly of the function for both cases.

Would it better if the strings were used directly in the function body?
I could replace the char* locals with #defines, for instance.

Amicalement,
-- 
Albert.

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

* [U-Boot] [RFC] ARM: prevent misaligned array inits
  2012-10-02 18:46 [U-Boot] [RFC] ARM: prevent misaligned array inits Albert ARIBAUD
  2012-10-02 19:13 ` Joakim Tjernlund
@ 2012-10-02 20:06 ` Stephen Warren
  2012-10-02 21:20   ` Albert ARIBAUD
  2012-10-02 21:42 ` Måns Rullgård
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Stephen Warren @ 2012-10-02 20:06 UTC (permalink / raw)
  To: u-boot

On 10/02/2012 12:46 PM, 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.

> +++ b/doc/README.arm-unaligned-accesses
...
> +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 -munified-access rule globally. This will prevent native

I assume that's meant to say -munaligned-access?

> +   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 m-unaligned-access.

s/m-/-m/

> +d) Relax the -munified-access rule only for for files susceptible to

I assume that's meant to say -munaligned-access?

> +   the local initialized array issue. This minimizes the quantity of
> +   code which can hide unwanted misaligned accesses.
> +
> +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.

I assume therefore that option (d) was chosen. Perhaps state this
explicitly?

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

* [U-Boot] [RFC] ARM: prevent misaligned array inits
  2012-10-02 20:06 ` Stephen Warren
@ 2012-10-02 21:20   ` Albert ARIBAUD
  0 siblings, 0 replies; 11+ messages in thread
From: Albert ARIBAUD @ 2012-10-02 21:20 UTC (permalink / raw)
  To: u-boot

Hi Stephen,

On Tue, 02 Oct 2012 14:06:53 -0600, Stephen Warren
<swarren@wwwdotorg.org> wrote:

> > +c) Relax the -munified-access rule globally. This will prevent native
> 
> I assume that's meant to say -munaligned-access?

> > +   until the target gets compiled with m-unaligned-access.
> 
> s/m-/-m/

> > +d) Relax the -munified-access rule only for for files susceptible to
> 
> I assume that's meant to say -munaligned-access?

Thanks for spotting these. Fixed in next round.

> > +   the local initialized array issue. This minimizes the quantity of
> > +   code which can hide unwanted misaligned accesses.
> > +
> > +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.
> 
> I assume therefore that option (d) was chosen. Perhaps state this
> explicitly?

Thanks for pointing out the ambiguity: indeed option d) is the one
chosen. Made explicit in next round.

Thanks again!

Amicalement,
-- 
Albert.

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

* [U-Boot] [RFC] ARM: prevent misaligned array inits
  2012-10-02 18:46 [U-Boot] [RFC] ARM: prevent misaligned array inits Albert ARIBAUD
  2012-10-02 19:13 ` Joakim Tjernlund
  2012-10-02 20:06 ` Stephen Warren
@ 2012-10-02 21:42 ` Måns Rullgård
  2012-10-03  7:29 ` Mark Marshall
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Måns Rullgård @ 2012-10-02 21:42 UTC (permalink / raw)
  To: u-boot

Albert ARIBAUD <albert.u.boot@aribaud.net> writes:

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

Why don't you just stop setting the damn strict alignment flag on ARMv6
and later instead this endless hacking around?

You really have only two sane choices here:

1. Keep strict alignment checking and build with -mno-unaligned-access.
2. Drop strict alignment checking and build with (implicit) -munaligned-access.

Option 2 gives faster, smaller code, so the choice should be an easy one
to make.

-- 
M?ns Rullg?rd
mans at mansr.com

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

* [U-Boot] [RFC] ARM: prevent misaligned array inits
  2012-10-02 18:46 [U-Boot] [RFC] ARM: prevent misaligned array inits Albert ARIBAUD
                   ` (2 preceding siblings ...)
  2012-10-02 21:42 ` Måns Rullgård
@ 2012-10-03  7:29 ` Mark Marshall
  2012-10-04 20:22   ` Albert ARIBAUD
  2012-10-04 12:02 ` Thierry Reding
  2012-10-04 19:10 ` Lucas Stach
  5 siblings, 1 reply; 11+ messages in thread
From: Mark Marshall @ 2012-10-03  7:29 UTC (permalink / raw)
  To: u-boot

I've just had a quick look at this in passing, but at least some of
these changes seem wrong to me.  For example, the code in
board/ti/omap2420h4/sys_info.c :: display_board_info
should be:

void display_board_info(u32 btype)
{
	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;

This produces smaller code and is probably what the original
author wanted the compiler to do.  I've only compile tested
this, not actually run it.

Original file:

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
   0 .text         000004b4  00000000  00000000  00000034  2**2
                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
   1 .data         00000000  00000000  00000000  000004e8  2**0
                   CONTENTS, ALLOC, LOAD, DATA
   2 .bss          00000000  00000000  00000000  000004e8  2**0
                   ALLOC
   3 .rodata.str1.1 00000072  00000000  00000000  000004e8  2**0
                   CONTENTS, ALLOC, LOAD, READONLY, DATA

After my changes:

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
   0 .text         000003ac  00000000  00000000  00000034  2**2
                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
   1 .data         00000000  00000000  00000000  000003e0  2**0
                   CONTENTS, ALLOC, LOAD, DATA
   2 .bss          00000000  00000000  00000000  000003e0  2**0
                   ALLOC
   3 .rodata       00000048  00000000  00000000  000003e0  2**2
                   CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
   4 .rodata.str1.1 00000047  00000000  00000000  00000428  2**0
                   CONTENTS, ALLOC, LOAD, READONLY, DATA


Regards,

Mark Marshall.

On 10/02/2012 08:46 PM, 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>
> ---
> Please test this patch with gcc 4.7 on boards which do data aborts
> or resets due to misaligned accesses and report result to me.
>
>   arch/arm/cpu/arm926ejs/orion5x/cpu.c |    4 +-
>   board/ti/omap2420h4/sys_info.c       |   24 ++++-----
>   common/Makefile                      |    3 ++
>   common/cmd_dfu.c                     |    2 +-
>   doc/README.arm-unaligned-accesses    |   95 ++++++++++++++++++++++++++++++++++
>   fs/fat/Makefile                      |    2 +
>   fs/ubifs/Makefile                    |    3 ++
>   lib/Makefile                         |    3 ++
>   8 files changed, 121 insertions(+), 15 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/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c
> index a9f7241..b462aa5 100644
> --- a/board/ti/omap2420h4/sys_info.c
> +++ b/board/ti/omap2420h4/sys_info.c
> @@ -237,18 +237,18 @@ 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_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;
>   	u32 cpu, rev, sec;
> diff --git a/common/Makefile b/common/Makefile
> index 125b2be..19b2130 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -227,6 +227,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..00fb1c0
> --- /dev/null
> +++ b/doc/README.arm-unaligned-accesses
> @@ -0,0 +1,95 @@
> +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 -munified-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 m-unaligned-access.
> +
> +d) Relax the -munified-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.
> +
> +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.
> +
> +Luckily, detecting files susceptible to the issue can be automated
> +through a filter/regexp/script which would recognize local char array
> +initializations. Automated 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 be informative only and shall not prevent
> +applying the patch.
> +
> +Upon a positive detection, either option -mno-unaligned-access is
> +applied (if not already) to the affected file(s), or if the array is a
> +hidden 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 45798de..44393ed 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -78,6 +78,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
>

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

* [U-Boot] [RFC] ARM: prevent misaligned array inits
  2012-10-02 20:05   ` Albert ARIBAUD
@ 2012-10-03  8:22     ` Joakim Tjernlund
  0 siblings, 0 replies; 11+ messages in thread
From: Joakim Tjernlund @ 2012-10-03  8:22 UTC (permalink / raw)
  To: u-boot

Albert ARIBAUD <albert.u.boot@aribaud.net> wrote on 2012/10/02 22:05:40:
>
> Hi Joakim,
>
> On Tue, 2 Oct 2012 21:13:39 +0200, Joakim Tjernlund
> <joakim.tjernlund@transmode.se> wrote:
>
> >
> >
> > >   *********************************************************************/
> > >  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_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 = "?";
> >
> > hmm, on ppc I think this will cause relocation entries which will build size.
> >
> >  Jocke
>
> Can you try it and post results with and without it? Sizes and if
> possible disassembly of the function for both cases.

Since you asked, I had to check :)
Did this:
#include <stdio.h>
#ifdef NO_RELOC
void display_board_info1(int 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[] = "?";
	printf("%s,%s,%s,%s,%s,%s,%s,%s,%s,%s,%s,%s\n",
	       cpu_2420, cpu_2422, cpu_2423, db_men, db_ip, mem_sdr,
	       mem_ddr, t_tst, t_emu, t_hs, t_gp, unk);
}
#else
void display_board_info2(int 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 = "?";
	printf("%s,%s,%s,%s,%s,%s,%s,%s,%s,%s,%s,%s\n",
	       cpu_2420, cpu_2422, cpu_2423, db_men, db_ip, mem_sdr,
	       mem_ddr, t_tst, t_emu, t_hs, t_gp, unk);
}
#endif
and built it with:
powerpc-softfloat_4.5.3-linux-gnu-gcc -O2 -fpic -mrelocatable ppc-str-reloc.c -c -DNO_RELOC -o no_reloc.o

and

powerpc-softfloat_4.5.3-linux-gnu-gcc -O2 -fpic -mrelocatable ppc-str-reloc.c -c -o reloc.o

Result is:
#> size reloc.o no_reloc.o    text	   data	    bss	    dec	    hex	filename
    248	     52	      0	    300	    12c	reloc.o
    538	      0	      0	    538	    21a	no_reloc.o

Turns out that gcc still uses const string ptrs which makes for really bad code.
So your char * conversion is better for ppc too given the way gcc operates

 Jocke

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

* [U-Boot] [RFC] ARM: prevent misaligned array inits
  2012-10-02 18:46 [U-Boot] [RFC] ARM: prevent misaligned array inits Albert ARIBAUD
                   ` (3 preceding siblings ...)
  2012-10-03  7:29 ` Mark Marshall
@ 2012-10-04 12:02 ` Thierry Reding
  2012-10-04 19:10 ` Lucas Stach
  5 siblings, 0 replies; 11+ messages in thread
From: Thierry Reding @ 2012-10-04 12:02 UTC (permalink / raw)
  To: u-boot

On Tue, Oct 02, 2012 at 08:46: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>
> ---
> Please test this patch with gcc 4.7 on boards which do data aborts
> or resets due to misaligned accesses and report result to me.

I've tested this on TEC, on top of Tom's tegra/next branch. Without the
patch, U-Boot hangs right after:

	Loading Device Tree to 010f9000, end 010ff2d4 ... OK

With the patch applied the system boots to the login prompt, so:

Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121004/c51258b7/attachment.pgp>

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

* [U-Boot] [RFC] ARM: prevent misaligned array inits
  2012-10-02 18:46 [U-Boot] [RFC] ARM: prevent misaligned array inits Albert ARIBAUD
                   ` (4 preceding siblings ...)
  2012-10-04 12:02 ` Thierry Reding
@ 2012-10-04 19:10 ` Lucas Stach
  5 siblings, 0 replies; 11+ messages in thread
From: Lucas Stach @ 2012-10-04 19:10 UTC (permalink / raw)
  To: u-boot

Hi Albert,

Am Dienstag, den 02.10.2012, 20:46 +0200 schrieb Albert ARIBAUD:
> 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>
> ---
> Please test this patch with gcc 4.7 on boards which do data aborts
> or resets due to misaligned accesses and report result to me.
> 
Although, as you know, I don't like the general direction in which this
is heading you get a

Tested-by: Lucas Stach <dev@lynxeye.de>

As it at least allows for a booting machine in various configurations on
my Colibri T20.

>  arch/arm/cpu/arm926ejs/orion5x/cpu.c |    4 +-
>  board/ti/omap2420h4/sys_info.c       |   24 ++++-----
>  common/Makefile                      |    3 ++
>  common/cmd_dfu.c                     |    2 +-
>  doc/README.arm-unaligned-accesses    |   95 ++++++++++++++++++++++++++++++++++
>  fs/fat/Makefile                      |    2 +
>  fs/ubifs/Makefile                    |    3 ++
>  lib/Makefile                         |    3 ++
>  8 files changed, 121 insertions(+), 15 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/board/ti/omap2420h4/sys_info.c b/board/ti/omap2420h4/sys_info.c
> index a9f7241..b462aa5 100644
> --- a/board/ti/omap2420h4/sys_info.c
> +++ b/board/ti/omap2420h4/sys_info.c
> @@ -237,18 +237,18 @@ 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_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;
>  	u32 cpu, rev, sec;
> diff --git a/common/Makefile b/common/Makefile
> index 125b2be..19b2130 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -227,6 +227,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..00fb1c0
> --- /dev/null
> +++ b/doc/README.arm-unaligned-accesses
> @@ -0,0 +1,95 @@
> +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 -munified-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 m-unaligned-access.
> +
> +d) Relax the -munified-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.
> +
> +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.
> +
> +Luckily, detecting files susceptible to the issue can be automated
> +through a filter/regexp/script which would recognize local char array
> +initializations. Automated 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 be informative only and shall not prevent
> +applying the patch.
> +
> +Upon a positive detection, either option -mno-unaligned-access is
> +applied (if not already) to the affected file(s), or if the array is a
> +hidden 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 45798de..44393ed 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -78,6 +78,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

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

* [U-Boot] [RFC] ARM: prevent misaligned array inits
  2012-10-03  7:29 ` Mark Marshall
@ 2012-10-04 20:22   ` Albert ARIBAUD
  0 siblings, 0 replies; 11+ messages in thread
From: Albert ARIBAUD @ 2012-10-04 20:22 UTC (permalink / raw)
  To: u-boot

Hi Mark,

On Wed, 3 Oct 2012 09:29:09 +0200, Mark Marshall
<mark.marshall@omicron.at> wrote:

> I've just had a quick look at this in passing, but at least some of
> these changes seem wrong to me.  For example, the code in
> board/ti/omap2420h4/sys_info.c :: display_board_info
> should be:
> 
> void display_board_info(u32 btype)
> {
> 	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;
> 
> This produces smaller code and is probably what the original
> author wanted the compiler to do.  I've only compile tested
> this, not actually run it.
> 
> Original file:
> 
> Sections:
> Idx Name          Size      VMA       LMA       File off  Algn
>    0 .text         000004b4  00000000  00000000  00000034  2**2
>                    CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
>    1 .data         00000000  00000000  00000000  000004e8  2**0
>                    CONTENTS, ALLOC, LOAD, DATA
>    2 .bss          00000000  00000000  00000000  000004e8  2**0
>                    ALLOC
>    3 .rodata.str1.1 00000072  00000000  00000000  000004e8  2**0
>                    CONTENTS, ALLOC, LOAD, READONLY, DATA
> 
> After my changes:
> 
> Sections:
> Idx Name          Size      VMA       LMA       File off  Algn
>    0 .text         000003ac  00000000  00000000  00000034  2**2
>                    CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
>    1 .data         00000000  00000000  00000000  000003e0  2**0
>                    CONTENTS, ALLOC, LOAD, DATA
>    2 .bss          00000000  00000000  00000000  000003e0  2**0
>                    ALLOC
>    3 .rodata       00000048  00000000  00000000  000003e0  2**2
>                    CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
>    4 .rodata.str1.1 00000047  00000000  00000000  00000428  2**0
>                    CONTENTS, ALLOC, LOAD, READONLY, DATA
> 

Thanks Mike. Indeed, there is a range of choices to replace the
original local char arrays: global arrays, static local arrays, 
char pointers, const char pointers, or even direct strings in code. I'd
originally gone for const chars because the idea was not optimizing but
getting rid of a compiler issue, however, with my toolchain, this led
to warnings about losing the const qualifier, so I went to simple char*.
I'll recheck with (const) static char[] and chose whetever gets the
best score.

> Regards,
> 
> Mark Marshall.

Amicalement,
-- 
Albert.

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

end of thread, other threads:[~2012-10-04 20:22 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-02 18:46 [U-Boot] [RFC] ARM: prevent misaligned array inits Albert ARIBAUD
2012-10-02 19:13 ` Joakim Tjernlund
2012-10-02 20:05   ` Albert ARIBAUD
2012-10-03  8:22     ` Joakim Tjernlund
2012-10-02 20:06 ` Stephen Warren
2012-10-02 21:20   ` Albert ARIBAUD
2012-10-02 21:42 ` Måns Rullgård
2012-10-03  7:29 ` Mark Marshall
2012-10-04 20:22   ` Albert ARIBAUD
2012-10-04 12:02 ` Thierry Reding
2012-10-04 19:10 ` Lucas Stach

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox