diff --git a/apps/sequence/sequence.h b/apps/sequence/sequence.h index 41a376973..d7f103890 100644 --- a/apps/sequence/sequence.h +++ b/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 diff --git a/apps/shared/continuous_function.h b/apps/shared/continuous_function.h index dbeff485b..612651231 100644 --- a/apps/shared/continuous_function.h +++ b/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) diff --git a/apps/shared/function.cpp b/apps/shared/function.cpp index 7311064ca..31b1e01e0 100644 --- a/apps/shared/function.cpp +++ b/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(); diff --git a/apps/shared/function.h b/apps/shared/function.h index f884e5505..7cfad9a11 100644 --- a/apps/shared/function.h +++ b/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 diff --git a/apps/shared/range_1D.h b/apps/shared/range_1D.h index dd9353391..58ef2537f 100644 --- a/apps/shared/range_1D.h +++ b/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; diff --git a/apps/shared/test/function_alignement.cpp b/apps/shared/test/function_alignement.cpp index deaf9393e..df9136b26 100644 --- a/apps/shared/test/function_alignement.cpp +++ b/apps/shared/test/function_alignement.cpp @@ -9,7 +9,7 @@ namespace Shared { template 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(); testAlignmentHandlingFor(); }