| Message ID | 201206080015.q580FL8H014115@ignucius.se.axis.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.