[apps/shared] Remove packed data members for RecordDataBuffer

Change-Id: I04ea5ccb4c15bda975bf5af178f07092c0387312

apps/graph/continuous_function_store.h

@@ -16,8 +16,8 @@ public:
     return recordSatisfyingTestAtIndex(i, &isFunctionActiveOfType, &plotType);
   }
   Shared::ExpiringPointer<Shared::ContinuousFunction> modelForRecord(Ion::Storage::Record record) const { return Shared::ExpiringPointer<Shared::ContinuousFunction>(static_cast<Shared::ContinuousFunction *>(privateModelForRecord(record))); }
-private:
   Ion::Storage::Record::ErrorStatus addEmptyModel() override;
+private:
   const char * modelExtension() const override { return Ion::Storage::funcExtension; }
   Shared::ExpressionModelHandle * setMemoizedModelAtIndex(int cacheIndex, Ion::Storage::Record record) const override;
   Shared::ExpressionModelHandle * memoizedModelAtIndex(int cacheIndex) const override;

apps/sequence/sequence.h

@@ -78,7 +78,7 @@ private:
   class RecordDataBuffer : public Shared::Function::RecordDataBuffer {
   public:
     RecordDataBuffer(KDColor color) :
-      Shared::Function::RecordDataBuffer(color),
+      Shared::Function::RecordDataBuffer(color, sizeof(RecordDataBuffer)),
       m_type(Type::Explicit),
       m_initialRank(0),
       m_initialConditionSizes{0,0}
@@ -102,9 +102,9 @@ private:
 #if __EMSCRIPTEN__
     // See comment about emscripten alignement 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] __attribute__((packed));
+    emscripten_align1_short m_initialConditionSizes[2];
 #else
-    uint16_t m_initialConditionSizes[2] __attribute__((packed));
+    uint16_t m_initialConditionSizes[2];
 #endif
   };
 

apps/shared/Makefile

@@ -85,3 +85,18 @@ app_shared_src = $(addprefix apps/shared/,\
 
 app_shared_src += $(app_shared_test_src)
 apps_src += $(app_shared_src)
+
+# The .cpp files could also be added to app_shared_test_src in their respective makefiles
+#     -> it would then be impossible to run the shared test alone
+app_shared_test_src += $(addprefix apps/graph/,\
+  continuous_function_store.cpp\
+)
+
+app_shared_test_src += $(addprefix apps/sequence/,\
+  sequence.cpp\
+  sequence_store.cpp\
+)
+
+tests_src += $(addprefix apps/shared/test/,\
+  function_alignement.cpp\
+)

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 __attribute__((packed)) RecordDataBuffer : public Function::RecordDataBuffer {
+  class RecordDataBuffer : public Function::RecordDataBuffer {
   public:
     RecordDataBuffer(KDColor color) :
-      Function::RecordDataBuffer(color),
+      Function::RecordDataBuffer(color, sizeof(RecordDataBuffer)),
       m_plotType(PlotType::Cartesian),
       m_domain(-INFINITY, INFINITY),
       m_displayDerivative(false)

apps/shared/function.cpp

@@ -68,6 +68,17 @@ 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

@@ -61,7 +61,7 @@ protected:
    *   creating dependency on uninitialized values. */
   class RecordDataBuffer {
   public:
-    RecordDataBuffer(KDColor color) : m_color(color), m_active(true) {}
+    RecordDataBuffer(KDColor color, size_t size);
     KDColor color() const {
       return KDColor::RGB16(m_color);
     }
@@ -77,9 +77,9 @@ protected:
      * version of uint16_t type to avoid producing an alignment error on the
      * emscripten platform. */
     static_assert(sizeof(emscripten_align1_short) == sizeof(uint16_t), "emscripten_align1_short should have the same size as uint16_t");
-    emscripten_align1_short m_color __attribute__((packed));
+    emscripten_align1_short m_color;
 #else
-    uint16_t m_color __attribute__((packed));
+    uint16_t m_color;
 #endif
     bool m_active;
   };

apps/shared/range_1D.h

@@ -11,10 +11,9 @@
 
 namespace Shared {
 
-/* This class is used in a DataBuffer of a Storage::Record. See comment in
- * Shared::Function::RecordDataBuffer about packing. */
+// This class is used in a DataBuffer of a Storage::Record
 
-class __attribute__((packed)) Range1D final {
+class 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. */
@@ -34,8 +33,8 @@ private:
 #if __EMSCRIPTEN__
     // See comment about emscripten alignement 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 __attribute__((packed));
-    emscripten_align1_float m_max __attribute__((packed));
+    emscripten_align1_float m_min;
+    emscripten_align1_float m_max;
 #else
   float m_min;
   float m_max;

apps/shared/test/function_alignement.cpp

@@ -0,0 +1,80 @@
+#include <quiz.h>
+#include "../continuous_function.h"
+#include "../../graph/continuous_function_store.h"
+#include "../../sequence/sequence.h"
+#include "../../sequence/sequence_store.h"
+
+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 */
+  KDColor color = fct->color();
+  (void) color; // Silence compilation warning about unused variable.
+}
+
+void interactWithRecordMember(Sequence::SequenceStore * store, Ion::Storage::Record rec) {
+  Sequence::Sequence * seq = store->modelForRecord(rec);
+  /* Setting Sequence type will write record member m_initialConditionSizes,
+   * which has a 2-byte alignment */
+  seq->setType(Sequence::Sequence::Type::SingleRecurrence);
+  interactWithBaseRecordMember<Sequence::Sequence>(seq);
+}
+
+void interactWithRecordMember(Graph::ContinuousFunctionStore * store, Ion::Storage::Record rec) {
+  ContinuousFunction * fct = store->modelForRecord(rec).pointer();
+  // Setting m_min from record member m_domain, which has a 2-byte alignment
+  fct->setTMin(3.0f);
+  interactWithBaseRecordMember<ContinuousFunction>(fct);
+}
+
+template<class T>
+Ion::Storage::Record createRecord(T * store) {
+  Ion::Storage::Record::ErrorStatus err = store->addEmptyModel();
+  assert(err == Ion::Storage::Record::ErrorStatus::None);
+  (void) err; // Silence compilation warning about unused variable.
+  return store->recordAtIndex(store->numberOfModels()-1);
+}
+
+template<class T>
+void testAlignmentHandlingFor() {
+  T store;
+  Ion::Storage * sharedStorage = Ion::Storage::sharedStorage();
+
+  sharedStorage->destroyAllRecords();
+  Ion::Storage::Record rec1 = createRecord<T>(&store);
+  // Evaluate the sequence shift compared to a 2-byte alignment
+  uintptr_t shift = reinterpret_cast<uintptr_t>(rec1.value().buffer) % 2;
+
+  /* Interact with an alignment sensitive member. A mishandled record alignment
+   * should throw an abort(alignment fault) exception */
+  interactWithRecordMember(&store, rec1);
+
+  sharedStorage->destroyAllRecords();
+  // Repeat the same process with a 1 byte record padding
+  Ion::Storage::Record::ErrorStatus err = sharedStorage->createRecordWithExtension("1", "1", "1", 1);
+  assert(err == Ion::Storage::Record::ErrorStatus::None);
+  (void) err; // Silence compilation warning about unused variable.
+
+  Ion::Storage::Record rec2 = createRecord<T>(&store);
+  shift += reinterpret_cast<uintptr_t>(rec2.value().buffer) % 2;
+  interactWithRecordMember(&store, rec2);
+
+  /* If fct1 and fct2 had different shifts, the test is able to detect a
+   * mishandled record alignment */
+  quiz_assert(shift == 1);
+
+  sharedStorage->destroyAllRecords();
+}
+
+QUIZ_CASE(alignment_handled_on_emscripten) {
+  /* 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). */
+  testAlignmentHandlingFor<Sequence::SequenceStore>();
+  testAlignmentHandlingFor<Graph::ContinuousFunctionStore>();
+}
+
+}