[apps/shared] Revert to pack record

It appeared that without the packed keyword, the compiler did not handle
access to unaligned record data members, which leads to crashes on the
device.

Change-Id: I401f075e7f62458a4733baa8d81983d4be34b730

apps/sequence/sequence.h

@@ -75,10 +75,10 @@ private:
   /* RecordDataBuffer is the layout of the data buffer of Record
    * representing a Sequence. See comment in
    * Shared::Function::RecordDataBuffer about packing. */
-  class RecordDataBuffer : public Shared::Function::RecordDataBuffer {
+  class __attribute__((packed)) RecordDataBuffer : public Shared::Function::RecordDataBuffer {
   public:
     RecordDataBuffer(KDColor color) :
-      Shared::Function::RecordDataBuffer(color, sizeof(RecordDataBuffer)),
+      Shared::Function::RecordDataBuffer(color),
       m_type(Type::Explicit),
       m_initialRank(0),
       m_initialConditionSizes{0,0}
@@ -100,7 +100,7 @@ private:
     Type m_type;
     uint8_t m_initialRank;
 #if __EMSCRIPTEN__
-    // See comment about emscripten alignement in Shared::Function::RecordDataBuffer
+    // See comment about emscripten alignment in Shared::Function::RecordDataBuffer
     static_assert(sizeof(emscripten_align1_short) == sizeof(uint16_t), "emscripten_align1_short should have the same size as uint16_t");
     emscripten_align1_short m_initialConditionSizes[2];
 #else

apps/shared/continuous_function.h

@@ -81,10 +81,10 @@ private:
   /* RecordDataBuffer is the layout of the data buffer of Record
    * representing a ContinuousFunction. See comment on
    * Shared::Function::RecordDataBuffer about packing. */
-  class RecordDataBuffer : public Function::RecordDataBuffer {
+  class __attribute__((packed)) RecordDataBuffer : public Function::RecordDataBuffer {
   public:
     RecordDataBuffer(KDColor color) :
-      Function::RecordDataBuffer(color, sizeof(RecordDataBuffer)),
+      Function::RecordDataBuffer(color),
       m_plotType(PlotType::Cartesian),
       m_domain(-INFINITY, INFINITY),
       m_displayDerivative(false)

apps/shared/function.cpp

@@ -68,17 +68,6 @@ int Function::nameWithArgument(char * buffer, size_t bufferSize) {
   return result;
 }
 
-Function::RecordDataBuffer::RecordDataBuffer(KDColor color, size_t size) {
-  /* Size is passed so that the entire derived RecordDataBuffer can be set to 0
-   * before initializing parameters. This is done in order to ensure any padding
-   * bits are set to 0 and prevent storage's CRC32 from depending on junk data. */
-  assert(size >= sizeof(*this));
-  memset(this, 0, size);
-  // Members must be initialized after memset
-  m_color = color;
-  m_active = true;
-}
-
 Function::RecordDataBuffer * Function::recordData() const {
   assert(!isNull());
   Ion::Storage::Record::Data d = value();

apps/shared/function.h

@@ -58,10 +58,17 @@ protected:
    * representing a Function. We want to avoid padding which would:
    * - increase the size of the storage file
    * - introduce junk memory zone which are then crc-ed in Storage::checksum
-   *   creating dependency on uninitialized values. */
-  class RecordDataBuffer {
+   *   creating dependency on uninitialized values.
+   * - complicate getters, setters and record handling
+   * In addition, Record::value() is a pointer to an address inside
+   * Ion::Storage::sharedStorage(), and it might be unaligned. We use the packed
+   * keyword to warn the compiler that it members are potentially unaligned
+   * (otherwise, the compiler can emit instructions that work only on aligned
+   * objects). It also solves the padding issue mentioned above.
+   */
+  class __attribute__((packed)) RecordDataBuffer {
   public:
-    RecordDataBuffer(KDColor color, size_t size);
+    RecordDataBuffer(KDColor color) : m_color(color), m_active(true) {}
     KDColor color() const {
       return KDColor::RGB16(m_color);
     }
@@ -69,9 +76,7 @@ protected:
     void setActive(bool active) { m_active = active; }
   private:
 #if __EMSCRIPTEN__
-    /* Record::value() is a pointer to an address inside
-     * Ion::Storage::sharedStorage(), and it might be unaligned. However, for
-     * emscripten memory representation, loads and stores must be aligned;
+    /* For emscripten memory representation, loads and stores must be aligned;
      * performing a normal load or store on an unaligned address can fail
      * silently. We thus use 'emscripten_align1_short' type, the unaligned
      * version of uint16_t type to avoid producing an alignment error on the

apps/shared/range_1D.h

@@ -11,9 +11,10 @@
 
 namespace Shared {
 
-// This class is used in a DataBuffer of a Storage::Record
+/* This class is used in a DataBuffer of a Storage::Record. See comment in
+ * Shared::Function::RecordDataBuffer about packing. */
 
-class Range1D final {
+class __attribute__((packed)) Range1D final {
 public:
   /* If m_min and m_max are too close, we cannot divide properly the range by
    * the number of pixels, which creates a drawing problem. */
@@ -31,7 +32,7 @@ public:
   static float defaultRangeLengthFor(float position);
 private:
 #if __EMSCRIPTEN__
-    // See comment about emscripten alignement in Shared::Function::RecordDataBuffer
+    // See comment about emscripten alignment in Shared::Function::RecordDataBuffer
     static_assert(sizeof(emscripten_align1_short) == sizeof(uint16_t), "emscripten_align1_short should have the same size as uint16_t");
     emscripten_align1_float m_min;
     emscripten_align1_float m_max;

apps/shared/test/function_alignement.cpp

@@ -9,7 +9,7 @@ namespace Shared {
 template<class F>
 void interactWithBaseRecordMember(F * fct) {
   /* Accessing Function record member m_color, which has a 2-byte alignment
-   * Only efffective in DEBUG=1, as there are no compiler optimizations */
+   * Only effective in DEBUG=1, as there are no compiler optimizations */
   KDColor color = fct->color();
   (void) color; // Silence compilation warning about unused variable.
 }
@@ -68,11 +68,11 @@ void testAlignmentHandlingFor() {
   sharedStorage->destroyAllRecords();
 }
 
-QUIZ_CASE(alignment_handled_on_emscripten) {
+QUIZ_CASE(alignment_handling) {
   /* This test main function is to crash if storage alignment is not handled
-   * properly on DEBUG and __EMSCRIPTEN__ modes only. It also ensures that the
-   * right test - load and store of differently-aligned objects - is performed
-   * (if storage/record implementations change for instance). */
+   * properly. It also ensures that the right test - load and store of
+   * differently-aligned objects - is performed (if storage/record
+   * implementations change for instance). */
   testAlignmentHandlingFor<Sequence::SequenceStore>();
   testAlignmentHandlingFor<Graph::ContinuousFunctionStore>();
 }