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

Mark Marshall mark.marshall at omicron.at
Wed Oct 3 09:29:09 CEST 2012


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



More information about the U-Boot mailing list