From 0a2ededfcfdfad7d424f39f970473b3f0a1076f2 Mon Sep 17 00:00:00 2001 From: Hugo Saint-Vignes Date: Thu, 4 Jun 2020 12:24:29 +0200 Subject: [PATCH] [apps/shared] Remove packed data members for RecordDataBuffer Change-Id: I04ea5ccb4c15bda975bf5af178f07092c0387312 --- apps/graph/continuous_function_store.h | 2 +- apps/sequence/sequence.h | 6 +- apps/shared/Makefile | 15 +++++ apps/shared/continuous_function.h | 4 +- apps/shared/function.cpp | 11 ++++ apps/shared/function.h | 6 +- apps/shared/range_1D.h | 9 ++- apps/shared/test/function_alignement.cpp | 80 ++++++++++++++++++++++++ 8 files changed, 119 insertions(+), 14 deletions(-) create mode 100644 apps/shared/test/function_alignement.cpp diff --git a/apps/graph/continuous_function_store.h b/apps/graph/continuous_function_store.h index fce291049..2670a08e8 100644 --- a/apps/graph/continuous_function_store.h +++ b/apps/graph/continuous_function_store.h @@ -16,8 +16,8 @@ public: return recordSatisfyingTestAtIndex(i, &isFunctionActiveOfType, &plotType); } Shared::ExpiringPointer modelForRecord(Ion::Storage::Record record) const { return Shared::ExpiringPointer(static_cast(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; diff --git a/apps/sequence/sequence.h b/apps/sequence/sequence.h index d8e2a5f6d..41a376973 100644 --- a/apps/sequence/sequence.h +++ b/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 }; diff --git a/apps/shared/Makefile b/apps/shared/Makefile index 4ffcd50df..c10dece7c 100644 --- a/apps/shared/Makefile +++ b/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\ +) diff --git a/apps/shared/continuous_function.h b/apps/shared/continuous_function.h index 612651231..dbeff485b 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 __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) diff --git a/apps/shared/function.cpp b/apps/shared/function.cpp index 31b1e01e0..7311064ca 100644 --- a/apps/shared/function.cpp +++ b/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(); diff --git a/apps/shared/function.h b/apps/shared/function.h index 52bdff733..f884e5505 100644 --- a/apps/shared/function.h +++ b/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; }; diff --git a/apps/shared/range_1D.h b/apps/shared/range_1D.h index 1557e82b6..dd9353391 100644 --- a/apps/shared/range_1D.h +++ b/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; diff --git a/apps/shared/test/function_alignement.cpp b/apps/shared/test/function_alignement.cpp new file mode 100644 index 000000000..deaf9393e --- /dev/null +++ b/apps/shared/test/function_alignement.cpp @@ -0,0 +1,80 @@ +#include +#include "../continuous_function.h" +#include "../../graph/continuous_function_store.h" +#include "../../sequence/sequence.h" +#include "../../sequence/sequence_store.h" + +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 */ + 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(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(fct); +} + +template +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 +void testAlignmentHandlingFor() { + T store; + Ion::Storage * sharedStorage = Ion::Storage::sharedStorage(); + + sharedStorage->destroyAllRecords(); + Ion::Storage::Record rec1 = createRecord(&store); + // Evaluate the sequence shift compared to a 2-byte alignment + uintptr_t shift = reinterpret_cast(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(&store); + shift += reinterpret_cast(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(); + testAlignmentHandlingFor(); +} + +} \ No newline at end of file