ros (de)serialization is not multi-arch safe

[tfoote]

From Peter Rudolph on ROS Users: https://code.ros.org/lurker/thread/20130303.105941.7106c801.en.html

He guys!
I tried to solved and it seems solved! First i will explain the problem itself: while de-/serializing unaligned pointercasts are done. normally this is not a problem! but since newer versions of arm-gcc use float support with options -mfloat-abi=hard/softfp, there are many possibilities to hit this alignment fault, because NEON instructions must be aligned! This is a very big problem... compiling ROS with -Wcast-align shows the potential lines! Solving this by setting "-mno-unaligned-access" as compiler-flag does NOT work! Every prebuilt-compiler i used seemed to ignore this switch.

example from www, demonstrates the error (compile it w/o any optimization(-O0), else compiler will re-align the pointercast, but this won't work for ROS, since everything is built w/ optimization(-O2)):

include <stdio.h>

int main(int argc, char* argv[])
{
char attribute ((aligned(32))) buffer[8] = { 0 };
float* fval_p;

fval_p = (float*)&buffer[1];
*fval_p = 0.1234;

printf("\nfloat at &buf[1] is %f\n", *fval_p);

return 0;

}

Build my own example from this, to check what works correctly and serious...

Only possibility was to hack the correct lines in roscpp_serialization. I only changed the lines from pointercasts to simple memcpy's and now everything seems to work fine! No alignment faults, no bus errors, no kernel exceptions! Tried a lightweight openni version, my own robot stack, and gmapping! navigation stack will follow!

Due to having no time now, patchfile will follow, too!

Regards,
sem23

[kartikmohta]

This seems to be the same as an old bug report: https://code.ros.org/trac/ros/ticket/2883
The first patch seems to be the one needed to prevent these alignment issues.

[trainman419]

+1

The ROS community has been waiting for this patch to get released for a long time. As ARM gets more powerful and more platforms start using arm hard-float, we need to take another look at applying this.

[dirk-thomas]

Duplicate of ros/roscpp_core#8

[sem23]

--- ./include/ros/serialization.h   2013-03-05 10:07:18.210946502 +0100
+++ ./patch/serialization.h 2013-03-05 10:03:51.195321502 +0100
@@ -170,6 +170,7 @@
   return Serializer::serializedLength(t);
 }
 
+#if (!(defined __ARMEL__ && defined __VFP_FP__))
 #define ROS_CREATE_SIMPLE_SERIALIZER(Type) \
   template<> struct Serializer \
   { \
@@ -188,6 +189,26 @@
       return sizeof(Type); \
     } \
   };
+#else
+#define ROS_CREATE_SIMPLE_SERIALIZER(Type) \
+  template<> struct Serializer \
+  { \
+    template inline static void write(Stream& stream, const Type v) \
+    { \
+      memcpy((void*)stream.advance(sizeof(v)), (const void*) &v, sizeof(v)); \
+    } \
+    \
+    template inline static void read(Stream& stream, Type& v) \
+    { \
+      memcpy((void*)&v, (const void*) stream.advance(sizeof(v)), sizeof(v)); \
+    } \
+    \
+    inline static uint32_t serializedLength(const Type&) \
+    { \
+      return sizeof(Type); \
+    } \
+  };
+#endif
 
 ROS_CREATE_SIMPLE_SERIALIZER(uint8_t);
 ROS_CREATE_SIMPLE_SERIALIZER(int8_t);

[dirk-thomas]

A patch was already committed to roscpp_core as references above. Please take a look to that patch and comment if you think it needs further tweaking.

[zeltner]

Thanks a lot for the patch, it solved a lot of my problems. However, I had to perform the following two changes to my ROS Groovy system to get the ROS service client tutorial working on my ARM platform:

1. __arm is not defined by my compilation environment, but __arm__ is, so I changed

#ifdef __arm

into

#if defined(__arm__) || defined(__arm)

2. I had issues with another unaligned access at service_server_link.cpp and could fix it with the following change:

--- ./roscpp/src/libros/service_server_link.cpp  2013-04-08 15:45:22.000000000 +0000
+++ patch/service_server_link.cpp 2013-04-08 15:46:01.047581231 +0000
@@ -190,7 +190,14 @@
     return;

   uint8_t ok = buffer[0];
+
+#if defined(__arm__) || defined(__arm)
+  uint32_t len;
+
+  memcpy(&len, buffer.get() + 1, sizeof(uint32_t));
+#else
   uint32_t len = *((uint32_t*)(buffer.get() + 1));
+#endif

   if (len > 1000000000)
   {

[sem23]

In gcc 4.8 they fixed something with the unaligned access... currently i
don't know if this fixes our problem too, but i will try to find out asap!
Regards,
Peter
Am 09.04.2013 02:35 schrieb "Stefan Zeltner" notifications@github.com:

Thanks a lot for the patch, it solved a lot of my problems. However, I had
to perform the following two changes to my ROS Groovy system to get the ROS
service client tutorialhttp://www.ros.org/wiki/ROS/Tutorials/WritingServiceClient%28c%2B%2B%29working on my ARM platform:

1. arm is not defined by my compilation environment, but __arm is, so
   I changed

#ifdef __arm

into

#if defined(arm) || defined(__arm)

2. I had issues with another unaligned access at service_server_link.cpp
   and could fix it with the following change:

--- ./roscpp/src/libros/service_server_link.cpp 2013-04-08 15:45:22.000000000 +0000
+++ patch/service_server_link.cpp 2013-04-08 15:46:01.047581231 +0000
@@ -190,7 +190,14 @@
return;

uint8_t ok = buffer[0];
+
+#if defined(arm) || defined(__arm)

• uint32_t len;

• memcpy(&len, buffer.get() + 1, sizeof(uint32_t));
  +#else
  uint32_t len = ((uint32_t)(buffer.get() + 1));
  +#endif

if (len > 1000000000)
{

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/184#issuecomment-16087800
.

[severin-lemaignan]

I confirm that on an cortex-a8 with gcc-4.4, __arm is not defined, but __arm__ is. So @zeltner proposal to replace #ifdef __arm by #if defined(__arm__) || defined(__arm) should be definitely adopted.

[dirk-thomas]

@severin-lemaignan Please consider to provide a pull request adding the additional check for __arm__.

[severin-lemaignan]

This has been actually fixed in indigo by this commit.