ARM: prevent misaligned array inits

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 <[email protected]>

---
V2: fix incorrect doc file name in dabort handler message

 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

arch/arm/lib/interrupts.c +0 -0

 	show_regs (pt_regs);
 	bad_mode ();
 }

doc/README.arm-unaligned-accesses new file +33 -0

+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:
+

doc/README.arm-unaligned-accesses new file +9 -0

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

Dear Albert Aribaud,

Quoted 7 lines · 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.

Quoted 3 lines

Signed-off-by Albert ARIBAUD <[email protected]>

---
V2: fix incorrect doc file name in dabort handler message

well, I can not see any change regarding the doc file ;)

Quoted 7 lines

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

--------------^^

Quoted 5 lines

 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>

arch/arm/lib/interrupts.c void do_prefetch_abort (struct pt_regs *pt_regs) · line 172

Quoted 10 lines

arch/arm/lib/interrupts.c +1 -1

index 74ff5ce..02ccb22 100644
@@ -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");

--------------------------------------------------------------------^^

Quoted 3 lines

arch/arm/lib/interrupts.c +0 -0

 	show_regs (pt_regs);
 	bad_mode ();
 }

<snip>

doc/README.arm-unaligned-accesses line 5

Quoted 11 lines

doc/README.arm-unaligned-accesses new file +5 -0

new file mode 100644
index 0000000..6f07436
@@ -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

Quoted 6 lines

doc/README.arm-unaligned-accesses new file +6 -0

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

doc/README.arm-unaligned-accesses line 18

Quoted 7 lines

doc/README.arm-unaligned-accesses new file +7 -0

+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

----------------------------------------------^^

doc/README.arm-unaligned-accesses line 25

Quoted 7 lines

doc/README.arm-unaligned-accesses new file +7 -0

+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

-----^^

doc/README.arm-unaligned-accesses line 33

Quoted 8 lines

doc/README.arm-unaligned-accesses new file +8 -0

+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?

Quoted 33 lines

doc/README.arm-unaligned-accesses new file +33 -0

+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:
+

doc/README.arm-unaligned-accesses line 76

Quoted 10 lines

doc/README.arm-unaligned-accesses new file +10 -0

+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

-------------------------------------------------------^

Quoted 9 lines

doc/README.arm-unaligned-accesses new file +9 -0

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

doc/README.arm-unaligned-accesses line 95

Quoted 10 lines

doc/README.arm-unaligned-accesses new file +10 -0

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

doc/README.arm-unaligned-accesses line 101

Quoted 6 lines

doc/README.arm-unaligned-accesses new file +6 -0

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

doc/README.arm-unaligned-accesses line 110

Quoted 9 lines

doc/README.arm-unaligned-accesses new file +9 -0

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

Hi Andreas,

Quoted 10 lines · Andreas Bie?mann

Dear Albert Aribaud,

Quoted 7 lines · 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.

Quoted 5 lines

Quoted 3 lines

Signed-off-by Albert ARIBAUD <[email protected]>

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

Quoted 11 lines

doc/README.arm-unaligned-accesses line 95

Quoted 5 lines

doc/README.arm-unaligned-accesses new file +5 -0

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

Quoted 2 lines

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?

Quoted 10 lines

doc/README.arm-unaligned-accesses line 101

Quoted 7 lines

doc/README.arm-unaligned-accesses new file +6 -0

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

Hi Albert,

Quoted 15 lines · Albert ARIBAUD

Hi Andreas,

Quoted 10 lines · Andreas Bie?mann

Dear Albert Aribaud,

Quoted 7 lines · 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.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Signed-off-by Albert ARIBAUD <[email protected]>

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

Well, that was I understood by the V2 comment ...

but V1 has:
---8<---

(quoted diff) +1 -1

-	printf ("data abort\n");
+	printf ("data abort\n\n    MAYBE you should read

doc/README.unaligned-access\n\n");
--->8---

and V2 has:
---8<---

(quoted diff) +1 -1

-	printf ("data abort\n");
+	printf ("data abort\n\n    MAYBE you should read

doc/README.unaligned-accesses\n\n");
--->8---

However the filename is 'doc/README.arm-unaligned-accesses'

<snip>

Quoted 14 lines

Quoted 13 lines

doc/README.arm-unaligned-accesses line 95

Quoted 5 lines

doc/README.arm-unaligned-accesses new file +5 -0

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

Quoted 4 lines

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?

Sounds way better.

Quoted 11 lines

Quoted 10 lines

doc/README.arm-unaligned-accesses line 101

Quoted 7 lines

doc/README.arm-unaligned-accesses new file +6 -0

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

Quoted 1 line

Not sure I understand what you mean about "ask[ing] the list instead".

I meant to discuss this on the list and just install the hook then (at least for the denx.de repositories).

Quoted 6 lines

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.

But you are right, so forget my comment.

best regards

Andreas Bie?mann

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 <[email protected]>

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 <[email protected]>

Quoted 7 lines · 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.

Quoted 1 line

Signed-off-by Albert ARIBAUD <[email protected]>

We unfortunately have a problem here. This ends up passing -mno-unaligned-access globally, for all platforms. We need something like the following:

Ensure that we only pass -mno-unaligned-access to ARMv7 platforms (where we must ensure this flag is passed so no using call-cc-option).

Signed-off-by; Tom Rini <[email protected]>

arch/arm/cpu/armv7/config.mk +0 -0

index 5407cb6..3c5ca23 100644

arch/arm/cpu/armv7/config.mk +1 -0

+
 ifneq ($(CONFIG_IMX_CONFIG),)
 ALL-y	+= $(obj)u-boot.imx
 endif

common/Makefile +2 -2

index a498367..33c606a 100644
@@ -233,8 +233,8 @@ $(obj)../tools/envcrc:
 	$(MAKE) -C ../tools
 
 # SEE README.arm-unaligned-accesses
-$(obj)hush.o: CFLAGS += -mno-unaligned-access
-$(obj)fdt_support.o: CFLAGS += -mno-unaligned-access
+$(obj)hush.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
+$(obj)fdt_support.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
 
 #########################################################################
 

fs/fat/Makefile +1 -1

index 5c4a2aa..02e6881 100644
@@ -40,7 +40,7 @@ $(LIB):	$(obj).depend $(OBJS)
 	$(call cmd_link_o_target, $(OBJS))
 
 # SEE README.arm-unaligned-accesses
-$(obj)file.o: CFLAGS += -mno-unaligned-access
+$(obj)file.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
 
 #########################################################################
 

fs/ubifs/Makefile +1 -1

index 71c40f2..bfe6874 100644
@@ -43,7 +43,7 @@ $(LIB):	$(obj).depend $(OBJS)
 	$(call cmd_link_o_target, $(OBJS))
 
 # SEE README.arm-unaligned-accesses
-$(obj)super.o: CFLAGS += -mno-unaligned-access
+$(obj)super.o: CFLAGS += $(PLATFORM_NO_UNALIGNED)
 
 #########################################################################
 

lib/Makefile +0 -0

index decc5ee..e44e045 100644

Hi Tom,

Quoted 9 lines · Tom Rini

Quoted 7 lines · 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.

Quoted 5 lines

Quoted 1 line

Signed-off-by Albert ARIBAUD <[email protected]>

We unfortunately have a problem here. This ends up passing -mno-unaligned-access globally, for all platforms. We need something like the following:

General remark: I'd strongly suggest not to copy-paste in a list reply somtheing that looks suspiciously like a (new) patch. It leads to this kind of things: http://patchwork.ozlabs.org/patch/190403/ which may not be what you wanted.

Quoted 10 lines

Ensure that we only pass -mno-unaligned-access to ARMv7 platforms (where we must ensure this flag is passed so no using call-cc-option).

Signed-off-by; Tom Rini <[email protected]>

arch/arm/cpu/armv7/config.mk +0 -0

index 5407cb6..3c5ca23 100644

arch/arm/cpu/armv7/config.mk PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_ARMV7) · line 39

Quoted 6 lines

arch/arm/cpu/armv7/config.mk +3 -0

@@ -34,6 +34,10 @@ PLATFORM_CPPFLAGS += $(PF_CPPFLAGS_ARMV7)
 # =========================================================================
 PF_RELFLAGS_SLB_AT := $(call cc-option,-mshort-load-bytes,$(call cc-option,-malignment-traps,))
 PLATFORM_RELFLAGS += $(PF_RELFLAGS_SLB_AT)
+
+# SEE README.arm-unaligned-accesses
+PLATFORM_NO_UNALIGNED := -mno-unaligned-access

I suspect that you meant

PLATFORM_NO_UNALIGNED := $(call cc-option,-mno-unaligned-access,)

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 <[email protected]>

---

Quoted 9 lines · 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 <[email protected]>

---

Tom,

As per our discussion, this patch should be placed in u-boot/next right above the 2012-10 release commit.

At this time, just after a git fech u-boot, I don't see the patch in next at all.

Do you want me to prepare an ad hoc branch with this patch then the current content of u-boot/next, above v2012.10-rc3, so that you just have to git reset --hard your next on it?

Amicalement,

Quoted 25 lines · Albert ARIBAUD

Quoted 9 lines · 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 <[email protected]>

---

Tom,

As per our discussion, this patch should be placed in u-boot/next right above the 2012-10 release commit.

At this time, just after a git fech u-boot, I don't see the patch in next at all.

Do you want me to prepare an ad hoc branch with this patch then the current content of u-boot/next, above v2012.10-rc3, so that you just have to git reset --hard your next on it?

Quoted 1 line

Amicalement,

As the questions about our usage of rebasing in 'next' have shown, it's not quite as cut and dry as I had hoped to rebase next at my whim (and long term, that's a good thing I think). My plan tomorrow (or today, depending on your local timezone) is to release v2012.10, apply this v6 patch and then bring in next to master, see that everything is still building and go from there.

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 <[email protected]>

---
V6: Make sure that gcc does not silently drop -mno-unaligned-access
if it does not support it.