Alignment Trap on ARM cpu

[mretallack]

Currently avian generates kernel warnings on our ARMv5 platform, for example:

Alignment trap: JVM (20722) PC=0xb6a3aa20 Instr=0xe5120004 Address=0xa5b75cb6 FSR 0x001

The makeClass method can sometime use a FixedSize that is not 32-bit aligned value. This means that when it is used in baseSize (machine.h) to calculate the field at the provided offset, the offset is not aligned.

Currently we are using kernel fixup to work around this (https://www.kernel.org/doc/Documentation/arm/mem_alignment [^]).

There appears to be two sections that causes this:

1. the generation of the types (creation of type-initializations.cpp)
2. when a class is loaded and parsed (in parseClass ).

We have created a patch to hopefully fix the problem correctly on our system and are currently testing it. It would be useful to get some feed back on the solution, for example is not going to break something?

Thanks in advance Mark

40-armv5-alignment.txt

[joshuawarner32]

Hi Mark,

I think the earliest we've tested avian with is an ARMv6 chip (a raspberry pi). Without looking into precisely what changed between those versions, I'm kind of surprised that it works there (I'm assuming) and not on ARMv5. That makes me wonder if it's actually broken on a lot of arm systems.

The commit looks suspicious; can you see if reverting it or checking out a version before it fixes your issue?: cbde34620cfe6de7498ae954cc8c7a35895e5fdc

Currently, I would expect this line to handle the alignment in the cases where it matters (i.e. where there's an array that comes after the fixed data): https://github.com/ReadyTalk/avian/blob/master/src/tools/type-generator/main.cpp#L843. Do you know why that's not working properly? Does the class it's breaking on not have an array component, and if that's the case, why are we trying to access anything after the end of the class? If there is an array component to the class, what size are it's elements (arrayElementSize)? Does ARMv5 have stricter alignment constraints, even on values of smaller (e.g. 2-byte) types?

You'll notice there's a commented-out line here: https://github.com/ReadyTalk/avian/blob/master/src/tools/type-generator/main.cpp#L846, that should accomplish roughly the same thing your patch does, only earlier in the process.

@dicej will be able to better tell you if either of those patches is likely to break things.

I'm personally more curious to see what class is actually breaking things. I suspect there's just a subtle logic error somewhere in the alignment that we should try to tackle first - but falling back to padding things to the word size should work (as long as it's done consistently, everywhere it needs to be done).

[mretallack]

Thanks for the reply.

The main difference between ARMv5 and ARMv6 in this area is that ARMv5 has no options to deal with unaligned access, but it appears that ARMv6 does (see [1]),

I will have a go a reverting the patch (cbde346) and seeing what that does.

Hopefully will be able to provide more info tomorrow.

[1] https://groups.google.com/forum/#!topic/beagleboard/S6ZwdHCZ1a8

[mretallack]

One of the classes that seems to be causing the issue is "innerClassReference" with a fixedSize of 18. This class does not appear to have the "arrayField" set so does not get aligned to 32bit. When baseSize is called from copiedSizeInWords this causes the fieldAtOffset call (line 1974 in machine.h) to be unaligned.

I will continue to investigate.

Mark.

[joshuawarner32]

Does this patch work for you:?

diff --git a/src/avian/machine.h b/src/avian/machine.h
index 40b627c..3011dd4 100644
--- a/src/avian/machine.h
+++ b/src/avian/machine.h
@@ -1987,11 +1987,14 @@ inline unsigned baseSize(Thread* t UNUSED, object o, GcClass* class_)
 {
   assertT(t, class_->fixedSize() >= BytesPerWord);

-  return ceilingDivide(class_->fixedSize(), BytesPerWord)
-         + ceilingDivide(class_->arrayElementSize()
-                         * fieldAtOffset<uintptr_t>(
-                               o, class_->fixedSize() - BytesPerWord),
-                         BytesPerWord);
+  unsigned size = ceilingDivide(class_->fixedSize(), BytesPerWord);
+  if (class_->arrayElementSize() > 0) {
+    size += ceilingDivide(class_->arrayElementSize()
+                       * fieldAtOffset<uintptr_t>(
+                             o, class_->fixedSize() - BytesPerWord),
+                       BytesPerWord);
+  }
+  return size;
 }

 object makeTrace(Thread* t, Processor::StackWalker* walker);

[mretallack]

Thanks for the patch. That has fixed it. Sorry about the delay. Wanted to run the patch overnight to make sure.