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

Andreas Bießmann andreas.devel at googlemail.com
Mon Oct 8 13:21:45 CEST 2012


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


More information about the U-Boot mailing list