[U-Boot] [PATCH V2] ARM: prevent misaligned array inits
Albert ARIBAUD
albert.u.boot at aribaud.net
Mon Oct 8 16:36:39 CEST 2012
Hi Andreas,
On Mon, 08 Oct 2012 13:21:45 +0200, "Andreas Bießmann"
<andreas.devel at 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 at 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,
--
Albert.
More information about the U-Boot
mailing list