#3767 new defect

Should all PPC BSPs build with -mstrict-align?

Reported by: Michael Davidsaver Owned by:
Priority: normal Milestone: 6.1
Component: arch/powerpc Version: 6
Severity: normal Keywords:
Cc: Blocked By:
Blocking:

Description

As requested by Gedare Bloom.

https://lists.rtems.org/pipermail/users/2019-July/033400.html

Based on the description in the GCC documentation, I would expect
this to be needed unless the target handles alignment exceptions
to fixup unaligned access (which RTEMS doesn't).

https://gcc.gnu.org/onlinedocs/gcc/RS_002f6000-and-PowerPC-Options.html#RS_002f6000-and-PowerPC-Options

It looks like some, but not all, RTEMS BSPs provide this flag.
With relevant exceptions being mvme5500 and beatnik.

https://github.com/RTEMS/rtems/search?q=-mstrict-align&type=Code

We're seeing a fault from a memcpy() when a copy of a double into an
unaligned byte buffer is optimized to the builtin.

char *buf = ...;
double value = ...;
memcpy(buf, &value, sizeof(value));

In the absence of -mstrict-align this memcpy() is sometimes ("-mcpu=7400 -O3")
optimized to a single "stfd" which faults because 'buf' is not 8 byte aligned.

I feel like there is a bug here. My question is whether this is the
builtin making an overzealous assumption about destination alignment,
or our usage of memcpy() being incorrect somehow. Should this be reported as a GCC bug as well?

Change History (9)

comment:1 Changed on Jul 17, 2019 at 6:20:12 PM by Gedare Bloom

Milestone: 5.1
Version: 5

I think this option likely should be enabled for all powerpc BSPs.

It should also be back-ported to open branches.

comment:2 Changed on Jul 18, 2019 at 6:04:23 AM by Sebastian Huber

I don't think this should be enabled for all BSPs. For example the e6500 based QorIQ variants don't need it. It needs to be added case by case.

comment:3 Changed on Jul 18, 2019 at 7:00:23 PM by Michael Davidsaver

I forgot to link to our analysis https://github.com/epics-base/epics-base/issues/29

The effect can be seen by compiling:

#include <string.h>
void test_double(char *dest, double src)
{
   memcpy(dest, &src, sizeof(src));
}

void test_int64(char *dest, long long src)
{
   memcpy(dest, &src, sizeof(src));
}

eg.

powerpc-rtems4.10-gcc -mcpu=7400 -g -O3 -o copyme.o -c copyme.c  && powerpc-rtems4.10-objdump -dS copyme.o

Then add -mstrict-align and see the difference.

I should also note that -mcpu=8540 (mvme3100) doesn't seem to need -mstrict-align. Not sure if this is implied for this target.

comment:4 Changed on Jul 19, 2019 at 5:27:56 AM by Sebastian Huber

The GCC -mcpu option changes the default values of other options such as -mstrict-align. You can have a look at the GCC sources: gcc/config/rs6000/rs6000-cpus.def

If you have problems with an alignment exception on a -mcpu=7400 based BSP, then simply add the -mstrict-align to this BSP. Other BSPs should remain as is.

comment:5 Changed on Dec 12, 2019 at 5:06:10 PM by Joel Sherrill

Given that this needs to be addressed on a per-BSP basis and was filed against the Beatnik, shouldn't the Beatnik have this option added?

Other BSPs would require investigation to know whether it is needed to add this option.

comment:6 Changed on Dec 12, 2019 at 5:17:07 PM by Joel Sherrill

Milestone: 5.16.1

I hesitate to close this since I believe that at least the Beatnik likely needs this added. And the options for every BSP need to be evaluated.

As a result, I am moving this to a new milestone. I think the example has to be compiled against the CPU CFLAGS used by every BSP. I tried to do this programmatically but the use of include's makes this a bit more difficult than a simple grep.

This depends on the version of gcc and the CPU flags used. This will require hand testing across all BSPs.

I am moving the milestone to 6.1 to keep it from being "won't fix" and hoping someone will perform the review.

comment:7 Changed on Dec 12, 2019 at 7:00:03 PM by Sebastian Huber

I think we should a test case for this so that everyone who cares to run the test suite will notice the issue.

comment:8 Changed on Dec 12, 2019 at 9:04:39 PM by Chris Johns

Version: 56

In the new build system?

comment:9 Changed on Dec 13, 2019 at 4:20:56 PM by Michael Davidsaver

I think a test case would be an excellent idea.

From looking into this a bit more, the '-mstrict-align' flag is overly conservative, and doesn't capture the granularity of the instructions which have alignment restrictions on a particular target.

eg. from the mpc7450 docs on the possible causes of an alignment exception

  • A floating-point load/store, stmw, stwcx., lmw, lwarx, eciwx, or ecowx instruction operand is not word-aligned.
  • A multiple/string load/store operation is attempted in little-endian mode
  • An operand of a dcbz instruction is on a page that is write-through or cache-inhibited for a virtual mode access.
  • An attempt to execute a dcbz instruction occurs when the cache is disabled or locked.

which I read as meaning plain 'stw' would work.

Note: See TracTickets for help on using tickets.