Patchwork [RFA:] Caveat for ARM in gcc-4.7/changes.html: unaligned accesses

login
register
mail settings
Submitter Hans-Peter Nilsson
Date June 8, 2012, 12:15 a.m.
Message ID <201206080015.q580FL8H014115@ignucius.se.axis.com>
Download mbox | patch
Permalink /patch/163696/
State New
Headers show

Comments

Hans-Peter Nilsson - June 8, 2012, 12:15 a.m.
(CC to ARM maintainer approving the original patch.)

I'm listing this under "caveats" rather than "improvements" and
before the current top ARM-related caveat (as this one is more
important :) because I don't see performance figures in the
context of the original patch (r178852) backing up this as an
improvement, and it caused sort-of a wild goose hunt for
applications causing unaligned accesses, until it was found to be
deliberately emitted by gcc-4.7 when configured for
arm-unknown-linux-gnueabi and passing "-marmv6".  Hence caveat.
Nomenclature taken from that patch; if prefer a different
spelling of the ARM variants, please tell how.  The "some source
codes" was in the analyzed case a strcpy of a five-byte string
(busybox/libbb/procps.c:365 'strcpy(filename_tail, "stat");' of
some unknown busybox-version).

An OS configured with unaligned accesses turned off (IIUC the
default for Linux/ARM), has the nice property that you easily
spot a certain class of bad codes.

Ok to commit?

Alternatively and IMHO preferably, there's reason enough to
revert the (new) default, including the gcc-4.7 branch.


brgds, H-P
Michael Hope - June 8, 2012, 2:42 a.m.
On 8 June 2012 12:15, Hans-Peter Nilsson <hans-peter.nilsson@axis.com> wrote:
> (CC to ARM maintainer approving the original patch.)
>
> I'm listing this under "caveats" rather than "improvements" and
> before the current top ARM-related caveat (as this one is more
> important :) because I don't see performance figures in the
> context of the original patch (r178852) backing up this as an
> improvement, and it caused sort-of a wild goose hunt for
> applications causing unaligned accesses, until it was found to be
> deliberately emitted by gcc-4.7 when configured for
> arm-unknown-linux-gnueabi and passing "-marmv6".  Hence caveat.
> Nomenclature taken from that patch; if prefer a different
> spelling of the ARM variants, please tell how.  The "some source
> codes" was in the analyzed case a strcpy of a five-byte string
> (busybox/libbb/procps.c:365 'strcpy(filename_tail, "stat");' of
> some unknown busybox-version).
>
> An OS configured with unaligned accesses turned off (IIUC the
> default for Linux/ARM), has the nice property that you easily
> spot a certain class of bad codes.
>
> Ok to commit?

The wording is scarier than I'd like.  Userspace and the Linux kernel
post 3.1 are fine.  Earlier kernels fail to start due to the kernel
turning off the hardware support and an interaction of
-fconserve-stack and -munaligned-access cause a char array to be
unaligned on the stack.

I don't agree that unaligned access is a sign of wrong code.  It's
very common when poking about in protocol buffers.  Using the hardware
support is better than the multi-byte read plus OR that GCC used to
do.

Where else have you seen this?

> Alternatively and IMHO preferably, there's reason enough to
> revert the (new) default, including the gcc-4.7 branch.
>
> Index: changes.html
> ===================================================================
> RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-4.7/changes.html,v
> retrieving revision 1.113
> diff -p -u -r1.113 changes.html
> --- changes.html        5 Jun 2012 11:03:53 -0000       1.113
> +++ changes.html        8 Jun 2012 00:01:09 -0000
> @@ -43,6 +43,16 @@
>
>     </li>
>
> +    <li>On ARM, when compiling for ARMv6 (but not ARMv6-M), ARMv7-A,
> +    ARMv7-R, or ARMv7-M

How about "ARMv6K and above"? or "ARMv6 and above, except for ARMv6-M"

> the default of the new option
> +    <code>-munaligned-accesses</code> is on, which for some source
> +    codes generates code that accesses memory on unaligned adresses.
> +    This will require the OS of those systems to enable such accesses
> +    (controlled by CP15 register c1, refer to ARM documentation).

The CPU has an unaligned access trap which is off by default.  The
kernel has to turn the trap on to cause the fault.

> +    Alternatively or for compatibility with OS versions that do not
> +    enable unaligned accesses, all codes has to be compiled with
> +    <code>-mno-unaligned-accesses</code>.</li>
> +
>     <li>Support on ARM for the legacy floating-point accelerator (FPA) and
>     the mixed-endian floating-point format that it used has been obsoleted.
>     The ports that still use this format have been obsoleted as well.
>
> brgds, H-P
Michael Hope - June 8, 2012, 3:50 a.m.
On 8 June 2012 15:20, Hans-Peter Nilsson <hans-peter.nilsson@axis.com> wrote:
>> From: Michael Hope <michael.hope@linaro.org>
>> Date: Fri, 8 Jun 2012 04:42:30 +0200
>
>> On 8 June 2012 12:15, Hans-Peter Nilsson <hans-peter.nilsson@axis.com> wrote:
>> > The "some source
>> > codes" was in the analyzed case a strcpy of a five-byte string
>> > (busybox/libbb/procps.c:365 'strcpy(filename_tail, "stat");' of
>> > some unknown busybox-version).
>> >
>> > An OS configured with unaligned accesses turned off (IIUC the
>> > default for Linux/ARM), has the nice property that you easily
>> > spot a certain class of bad codes.
>> >
>> > Ok to commit?
>>
>> The wording is scarier than I'd like.  Userspace and the Linux kernel
>> post 3.1 are fine.
>
> So the default for ALIGNMENT_TRAP changed in >3.1?

ALIGNMENT_TRAP is on by default but the early boot time trap is now
conditional on the architecture level:

__enable_mmu:
#if defined(CONFIG_ALIGNMENT_TRAP) && __LINUX_ARM_ARCH__ < 6
	orr	r0, r0, #CR_A
#else
	bic	r0, r0, #CR_A
#endif

>>  Earlier kernels fail to start due to the kernel
>> turning off the hardware support and an interaction of
>> -fconserve-stack and -munaligned-access cause a char array to be
>> unaligned on the stack.
>>
>> I don't agree that unaligned access is a sign of wrong code.  It's
>> very common when poking about in protocol buffers.  Using the hardware
>> support is better than the multi-byte read plus OR that GCC used to
>> do.
>
> Totally disagree.  Code that e.g. casts unaligned char * to int *
> and dereferences that, is crappy and unportable and fails
> without OS-configury choice on some platforms, and is also
> avoidable.  But hopefully that wasn't what you meant.

I mean direct access via helpers:

inline u16 get_u16(const char *p) {
#ifdef __ARM_FEATURE_UNALIGNED
  return ntohs(*(const u16 *)p);
#else
...
}
...
  u16 checksum = get_u16(buffer + CHECKSUM_OFFSET);

>> Where else have you seen this?
>
> I don't get the "else".  I've seen the unaligned accesses from
> userland code as quoted above.  There were other (userland)
> places in that build-tree that I'm not going to check, as this
> was (again) GCC-generated code.  There were no other misaligned
> accesses on that system; not from the kernel, not from userspace.

You're suggesting we disable an optimisation.  The combination of
older Linux ARM kernels and GCC 4.7 gives a faulty kernel.  In all
other cases it should be an improvement.  Have you seen other
combinations where there's a fault when this feature is turned on?

-- Michael
Gerald Pfeifer - June 10, 2012, 10:27 p.m.
This is only a review wearing my web hat; it is orthogonal to the
discussion with the ARM guys. ;-)

On Fri, 8 Jun 2012, Hans-Peter Nilsson wrote:
> +    <li>On ARM, when compiling for ARMv6 (but not ARMv6-M), ARMv7-A,
> +    ARMv7-R, or ARMv7-M, the default of the new option
> +    <code>-munaligned-accesses</code> is on, which for some source

How about "the new option...is active by default"?

> +    This will require the OS of those systems to enable such accesses

Omit "of those systems" and spell out "operating system"?

> +    (controlled by CP15 register c1, refer to ARM documentation).

<code>c1</cdoe>

> +    Alternatively or for compatibility with OS versions that do not
> +    enable unaligned accesses, all codes has to be compiled with

"enable" -> "support"

"all code has"

Gerald

Patch

Index: changes.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-4.7/changes.html,v
retrieving revision 1.113
diff -p -u -r1.113 changes.html
--- changes.html	5 Jun 2012 11:03:53 -0000	1.113
+++ changes.html	8 Jun 2012 00:01:09 -0000
@@ -43,6 +43,16 @@ 
 
     </li>
 
+    <li>On ARM, when compiling for ARMv6 (but not ARMv6-M), ARMv7-A,
+    ARMv7-R, or ARMv7-M, the default of the new option
+    <code>-munaligned-accesses</code> is on, which for some source
+    codes generates code that accesses memory on unaligned adresses.
+    This will require the OS of those systems to enable such accesses
+    (controlled by CP15 register c1, refer to ARM documentation).
+    Alternatively or for compatibility with OS versions that do not
+    enable unaligned accesses, all codes has to be compiled with
+    <code>-mno-unaligned-accesses</code>.</li>
+
     <li>Support on ARM for the legacy floating-point accelerator (FPA) and
     the mixed-endian floating-point format that it used has been obsoleted.
     The ports that still use this format have been obsoleted as well.