From e04a1342cab2e6c0ac0c37f585232dbdd09d7b54 Mon Sep 17 00:00:00 2001 From: Andy Shaw Date: Tue, 21 May 2024 14:53:06 +0100 Subject: [PATCH] Possible fix for Hard Fault errors when assigning values to arrays See: https://github.com/Duet3D/RepRapFirmware/issues/1008 --- src/Platform/ArrayHandle.cpp | 40 +++++++++++++++++++++++++++--------- src/Platform/ArrayHandle.h | 4 ++-- 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/src/Platform/ArrayHandle.cpp b/src/Platform/ArrayHandle.cpp index a0c9d702da..c4a2179517 100644 --- a/src/Platform/ArrayHandle.cpp +++ b/src/Platform/ArrayHandle.cpp @@ -64,17 +64,31 @@ void ArrayHandle::AssignElement(size_t index, ExpressionValue &val) THROWS(GCode void ArrayHandle::AssignIndexed(const ExpressionValue& ev, size_t numIndices, const uint32_t *indices) THROWS(GCodeException) { WriteLocker locker(Heap::heapLock); // prevent other tasks modifying the heap - InternalAssignIndexed(ev, numIndices, indices); + // We may need to allocate heap space to complete this assignment. However this may result + // in garbage collection moving some of the storage used for the array, which may invalidate + // pointers to that storage. To avoid this we restart the assigment process (and so re calculate any + // pointers) if we have needed to allocate storage. The previously alocated storage is then provided + // via the newStorage variable. + ArrayHandle newStorage; + while (!InternalAssignIndexed(ev, numIndices, indices, newStorage)) + { + } } // Make an array unique and assign an element, possibly nested -void ArrayHandle::InternalAssignIndexed(const ExpressionValue& ev, size_t numIndices, const uint32_t *indices) THROWS(GCodeException) +// return true if the assigment completed, false if we had to allocate new stoarge and so need to be +// called again to complete the operation +bool ArrayHandle::InternalAssignIndexed(const ExpressionValue& ev, size_t numIndices, const uint32_t *indices, ArrayHandle& newStorage) THROWS(GCodeException) { if (indices[0] >= GetNumElements()) { throw GCodeException("array index out of bounds"); } - MakeUnique(); + if (!MakeUnique(newStorage)) + { + // we needed to allocate new storage, so unwind the stack and start again + return false; + } ArrayStorageSpace * const aSpace = reinterpret_cast(slotPtr->storage); if (numIndices == 1) { @@ -86,10 +100,9 @@ void ArrayHandle::InternalAssignIndexed(const ExpressionValue& ev, size_t numInd } else { - // Note that slotPtr->storage and hence aSpace may move when we call InternalAssignIndexed recursively, but the handle won't move - //?? the following doesn't allow for that! - aSpace->elements[indices[0]].ahVal.InternalAssignIndexed(ev, numIndices - 1, indices + 1); + return aSpace->elements[indices[0]].ahVal.InternalAssignIndexed(ev, numIndices - 1, indices + 1, newStorage); } + return true; } // Get the number of elements in a heap array @@ -182,7 +195,8 @@ const ArrayHandle& ArrayHandle::IncreaseRefCount() const noexcept } // Make this handle refer to non-shared array (the individual elements may be shared). Caller must already own a read lock on the heap. -void ArrayHandle::MakeUnique() THROWS(GCodeException) +// return true if we completed the operation, false if we need to be called again +bool ArrayHandle::MakeUnique(ArrayHandle& ah2) THROWS(GCodeException) { #if CHECK_HEAP_LOCKED Heap::heapLock.CheckHasWriteLock(); @@ -191,9 +205,13 @@ void ArrayHandle::MakeUnique() THROWS(GCodeException) { const ArrayStorageSpace * const aSpace = reinterpret_cast(slotPtr->storage); const size_t count = aSpace->count; - - ArrayHandle ah2; - ah2.Allocate(count); + // Have we previously allocated the needed storage? + if (ah2.slotPtr == nullptr) + { + // no so allocate the storage and return so we can be called again + ah2.Allocate(count); + return false; + } ArrayStorageSpace * const aSpace2 = reinterpret_cast(ah2.slotPtr->storage); for (size_t i = 0; i < count; ++i) { @@ -202,7 +220,9 @@ void ArrayHandle::MakeUnique() THROWS(GCodeException) --slotPtr->refCount; slotPtr = ah2.slotPtr; + ah2.slotPtr = nullptr; } + return true; } // AutoArrayHandle members diff --git a/src/Platform/ArrayHandle.h b/src/Platform/ArrayHandle.h index 847db64471..2ee7df0938 100644 --- a/src/Platform/ArrayHandle.h +++ b/src/Platform/ArrayHandle.h @@ -39,8 +39,8 @@ class ArrayHandle Heap::IndexSlot * null slotPtr; private: - void MakeUnique() THROWS(GCodeException); - void InternalAssignIndexed(const ExpressionValue& ev, size_t numIndices, const uint32_t *indices) THROWS(GCodeException) pre(numIndeces != 0); + bool MakeUnique(ArrayHandle& ah2) THROWS(GCodeException); + bool InternalAssignIndexed(const ExpressionValue& ev, size_t numIndices, const uint32_t *indices, ArrayHandle &ah2) THROWS(GCodeException) pre(numIndeces != 0); }; // Version of ArrayHandle that updates the reference counts automatically