From e5a82da26e29560f3e7121b600a12f8acd6a5a3f Mon Sep 17 00:00:00 2001 From: Jordi Vaquero Date: Wed, 11 Sep 2019 00:11:27 +0200 Subject: [PATCH] cpu, mem: Changing AtomicOpFunctor* for unique_ptr This change is based on modify the way we move the AtomicOpFunctor* through gem5 in order to mantain proper ownership of the object and ensuring its destruction when it is no longer used. Doing that we fix at the same time a memory leak in Request.hh where we were assigning a new AtomicOpFunctor* without destroying the previous one. This change creates a new type AtomicOpFunctor_ptr as a std::unique_ptr and move its ownership as needed. Except for its only usage when AtomicOpFunc() is called. Change-Id: Ic516f9d8217cb1ae1f0a19500e5da0336da9fd4f Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/20919 Reviewed-by: Andreas Sandberg Maintainer: Andreas Sandberg Tested-by: kokoro --- src/arch/generic/memhelpers.hh | 14 ++++++++------ src/base/types.hh | 2 ++ src/cpu/base_dyn_inst.hh | 7 ++++--- src/cpu/checker/cpu.hh | 2 +- src/cpu/exec_context.hh | 4 ++-- src/cpu/minor/exec_context.hh | 4 ++-- src/cpu/minor/lsq.cc | 4 ++-- src/cpu/minor/lsq.hh | 2 +- src/cpu/o3/cpu.hh | 4 ++-- src/cpu/o3/lsq.hh | 15 ++++++++------- src/cpu/o3/lsq_impl.hh | 4 ++-- src/cpu/simple/atomic.cc | 4 ++-- src/cpu/simple/atomic.hh | 2 +- src/cpu/simple/base.hh | 4 ++-- src/cpu/simple/exec_context.hh | 8 ++++---- src/cpu/simple/timing.cc | 5 +++-- src/cpu/simple/timing.hh | 2 +- src/mem/request.hh | 30 ++++++++++++------------------ 18 files changed, 59 insertions(+), 58 deletions(-) diff --git a/src/arch/generic/memhelpers.hh b/src/arch/generic/memhelpers.hh index 7fd4f70de..fa1af2694 100644 --- a/src/arch/generic/memhelpers.hh +++ b/src/arch/generic/memhelpers.hh @@ -125,15 +125,16 @@ writeMemAtomic(XC *xc, Trace::InstRecord *traceData, const MemT &mem, template Fault amoMemAtomic(XC *xc, Trace::InstRecord *traceData, MemT &mem, Addr addr, - Request::Flags flags, AtomicOpFunctor *amo_op) + Request::Flags flags, AtomicOpFunctor *_amo_op) { - assert(amo_op); + assert(_amo_op); // mem will hold the previous value at addr after the AMO completes memset(&mem, 0, sizeof(mem)); + AtomicOpFunctorPtr amo_op = AtomicOpFunctorPtr(_amo_op); Fault fault = xc->amoMem(addr, (uint8_t *)&mem, sizeof(MemT), flags, - amo_op); + std::move(amo_op)); if (fault == NoFault) { mem = TheISA::gtoh(mem); @@ -147,10 +148,11 @@ amoMemAtomic(XC *xc, Trace::InstRecord *traceData, MemT &mem, Addr addr, template Fault initiateMemAMO(XC *xc, Trace::InstRecord *traceData, Addr addr, MemT& mem, - Request::Flags flags, AtomicOpFunctor *amo_op) + Request::Flags flags, AtomicOpFunctor *_amo_op) { - assert(amo_op); - return xc->initiateMemAMO(addr, sizeof(MemT), flags, amo_op); + assert(_amo_op); + AtomicOpFunctorPtr amo_op = AtomicOpFunctorPtr(_amo_op); + return xc->initiateMemAMO(addr, sizeof(MemT), flags, std::move(amo_op)); } #endif diff --git a/src/base/types.hh b/src/base/types.hh index d99384529..453309416 100644 --- a/src/base/types.hh +++ b/src/base/types.hh @@ -259,6 +259,8 @@ struct TypedAtomicOpFunctor : public AtomicOpFunctor virtual void execute(T * p) = 0; }; +typedef std::unique_ptr AtomicOpFunctorPtr; + enum ByteOrder { BigEndianByteOrder, LittleEndianByteOrder diff --git a/src/cpu/base_dyn_inst.hh b/src/cpu/base_dyn_inst.hh index de76559fb..4b4b05c1d 100644 --- a/src/cpu/base_dyn_inst.hh +++ b/src/cpu/base_dyn_inst.hh @@ -311,7 +311,7 @@ class BaseDynInst : public ExecContext, public RefCounted const std::vector& byteEnable = std::vector()); Fault initiateMemAMO(Addr addr, unsigned size, Request::Flags flags, - AtomicOpFunctor *amo_op); + AtomicOpFunctorPtr amo_op); /** True if the DTB address translation has started. */ bool translationStarted() const { return instFlags[TranslationStarted]; } @@ -986,7 +986,7 @@ template Fault BaseDynInst::initiateMemAMO(Addr addr, unsigned size, Request::Flags flags, - AtomicOpFunctor *amo_op) + AtomicOpFunctorPtr amo_op) { // atomic memory instructions do not have data to be written to memory yet // since the atomic operations will be executed directly in cache/memory. @@ -995,7 +995,8 @@ BaseDynInst::initiateMemAMO(Addr addr, unsigned size, // memory return cpu->pushRequest( dynamic_cast(this), - /* atomic */ false, nullptr, size, addr, flags, nullptr, amo_op); + /* atomic */ false, nullptr, size, addr, flags, nullptr, + std::move(amo_op)); } #endif // __CPU_BASE_DYN_INST_HH__ diff --git a/src/cpu/checker/cpu.hh b/src/cpu/checker/cpu.hh index 440fe81b5..8db6aa376 100644 --- a/src/cpu/checker/cpu.hh +++ b/src/cpu/checker/cpu.hh @@ -565,7 +565,7 @@ class CheckerCPU : public BaseCPU, public ExecContext override; Fault amoMem(Addr addr, uint8_t* data, unsigned size, - Request::Flags flags, AtomicOpFunctor *amo_op) override + Request::Flags flags, AtomicOpFunctorPtr amo_op) override { panic("AMO is not supported yet in CPU checker\n"); } diff --git a/src/cpu/exec_context.hh b/src/cpu/exec_context.hh index b294387e2..a2b392492 100644 --- a/src/cpu/exec_context.hh +++ b/src/cpu/exec_context.hh @@ -270,7 +270,7 @@ class ExecContext { */ virtual Fault amoMem(Addr addr, uint8_t *data, unsigned int size, Request::Flags flags, - AtomicOpFunctor *amo_op) + AtomicOpFunctorPtr amo_op) { panic("ExecContext::amoMem() should be overridden\n"); } @@ -281,7 +281,7 @@ class ExecContext { */ virtual Fault initiateMemAMO(Addr addr, unsigned int size, Request::Flags flags, - AtomicOpFunctor *amo_op) + AtomicOpFunctorPtr amo_op) { panic("ExecContext::initiateMemAMO() should be overridden\n"); } diff --git a/src/cpu/minor/exec_context.hh b/src/cpu/minor/exec_context.hh index 1871e2479..87787f011 100644 --- a/src/cpu/minor/exec_context.hh +++ b/src/cpu/minor/exec_context.hh @@ -133,11 +133,11 @@ class ExecContext : public ::ExecContext Fault initiateMemAMO(Addr addr, unsigned int size, Request::Flags flags, - AtomicOpFunctor *amo_op) override + AtomicOpFunctorPtr amo_op) override { // AMO requests are pushed through the store path return execute.getLSQ().pushRequest(inst, false /* amo */, nullptr, - size, addr, flags, nullptr, amo_op); + size, addr, flags, nullptr, std::move(amo_op)); } RegVal diff --git a/src/cpu/minor/lsq.cc b/src/cpu/minor/lsq.cc index 1e5e89647..629d89dc6 100644 --- a/src/cpu/minor/lsq.cc +++ b/src/cpu/minor/lsq.cc @@ -1573,7 +1573,7 @@ LSQ::needsToTick() Fault LSQ::pushRequest(MinorDynInstPtr inst, bool isLoad, uint8_t *data, unsigned int size, Addr addr, Request::Flags flags, - uint64_t *res, AtomicOpFunctor *amo_op, + uint64_t *res, AtomicOpFunctorPtr amo_op, const std::vector& byteEnable) { assert(inst->translationFault == NoFault || inst->inLSQ); @@ -1635,7 +1635,7 @@ LSQ::pushRequest(MinorDynInstPtr inst, bool isLoad, uint8_t *data, request->request->setVirt(0 /* asid */, addr, size, flags, cpu.dataMasterId(), /* I've no idea why we need the PC, but give it */ - inst->pc.instAddr(), amo_op); + inst->pc.instAddr(), std::move(amo_op)); request->request->setByteEnable(byteEnable); requests.push(request); diff --git a/src/cpu/minor/lsq.hh b/src/cpu/minor/lsq.hh index a7c7cb632..c4baad826 100644 --- a/src/cpu/minor/lsq.hh +++ b/src/cpu/minor/lsq.hh @@ -708,7 +708,7 @@ class LSQ : public Named * the LSQ */ Fault pushRequest(MinorDynInstPtr inst, bool isLoad, uint8_t *data, unsigned int size, Addr addr, Request::Flags flags, - uint64_t *res, AtomicOpFunctor *amo_op, + uint64_t *res, AtomicOpFunctorPtr amo_op, const std::vector& byteEnable = std::vector()); diff --git a/src/cpu/o3/cpu.hh b/src/cpu/o3/cpu.hh index ac917dba9..b06182d43 100644 --- a/src/cpu/o3/cpu.hh +++ b/src/cpu/o3/cpu.hh @@ -713,13 +713,13 @@ class FullO3CPU : public BaseO3CPU /** CPU pushRequest function, forwards request to LSQ. */ Fault pushRequest(const DynInstPtr& inst, bool isLoad, uint8_t *data, unsigned int size, Addr addr, Request::Flags flags, - uint64_t *res, AtomicOpFunctor *amo_op = nullptr, + uint64_t *res, AtomicOpFunctorPtr amo_op = nullptr, const std::vector& byteEnable = std::vector()) { return iew.ldstQueue.pushRequest(inst, isLoad, data, size, addr, - flags, res, amo_op, byteEnable); + flags, res, std::move(amo_op), byteEnable); } /** CPU read function, forwards read to LSQ. */ diff --git a/src/cpu/o3/lsq.hh b/src/cpu/o3/lsq.hh index cc14ae423..6225c507d 100644 --- a/src/cpu/o3/lsq.hh +++ b/src/cpu/o3/lsq.hh @@ -299,7 +299,7 @@ class LSQ const Request::Flags _flags; std::vector _byteEnable; uint32_t _numOutstandingPackets; - AtomicOpFunctor *_amo_op; + AtomicOpFunctorPtr _amo_op; protected: LSQUnit* lsqUnit() { return &_port; } LSQRequest(LSQUnit* port, const DynInstPtr& inst, bool isLoad) : @@ -318,7 +318,7 @@ class LSQ const Addr& addr, const uint32_t& size, const Request::Flags& flags_, PacketDataPtr data = nullptr, uint64_t* res = nullptr, - AtomicOpFunctor* amo_op = nullptr) + AtomicOpFunctorPtr amo_op = nullptr) : _state(State::NotIssued), _senderState(nullptr), numTranslatedFragments(0), numInTranslationFragments(0), @@ -326,7 +326,7 @@ class LSQ _res(res), _addr(addr), _size(size), _flags(flags_), _numOutstandingPackets(0), - _amo_op(amo_op) + _amo_op(std::move(amo_op)) { flags.set(Flag::IsLoad, isLoad); flags.set(Flag::WbStore, @@ -412,7 +412,8 @@ class LSQ isAnyActiveElement(byteEnable.begin(), byteEnable.end())) { auto request = std::make_shared(_inst->getASID(), addr, size, _flags, _inst->masterId(), - _inst->instAddr(), _inst->contextId(), _amo_op); + _inst->instAddr(), _inst->contextId(), + std::move(_amo_op)); if (!byteEnable.empty()) { request->setByteEnable(byteEnable); } @@ -721,9 +722,9 @@ class LSQ const Request::Flags& flags_, PacketDataPtr data = nullptr, uint64_t* res = nullptr, - AtomicOpFunctor* amo_op = nullptr) : + AtomicOpFunctorPtr amo_op = nullptr) : LSQRequest(port, inst, isLoad, addr, size, flags_, data, res, - amo_op) {} + std::move(amo_op)) {} inline virtual ~SingleDataRequest() {} virtual void initiateTranslation(); @@ -1032,7 +1033,7 @@ class LSQ Fault pushRequest(const DynInstPtr& inst, bool isLoad, uint8_t *data, unsigned int size, Addr addr, Request::Flags flags, - uint64_t *res, AtomicOpFunctor *amo_op, + uint64_t *res, AtomicOpFunctorPtr amo_op, const std::vector& byteEnable); /** The CPU pointer. */ diff --git a/src/cpu/o3/lsq_impl.hh b/src/cpu/o3/lsq_impl.hh index e885e6172..c2d5e90b4 100644 --- a/src/cpu/o3/lsq_impl.hh +++ b/src/cpu/o3/lsq_impl.hh @@ -687,7 +687,7 @@ template Fault LSQ::pushRequest(const DynInstPtr& inst, bool isLoad, uint8_t *data, unsigned int size, Addr addr, Request::Flags flags, - uint64_t *res, AtomicOpFunctor *amo_op, + uint64_t *res, AtomicOpFunctorPtr amo_op, const std::vector& byteEnable) { // This comming request can be either load, store or atomic. @@ -717,7 +717,7 @@ LSQ::pushRequest(const DynInstPtr& inst, bool isLoad, uint8_t *data, size, flags, data, res); } else { req = new SingleDataRequest(&thread[tid], inst, isLoad, addr, - size, flags, data, res, amo_op); + size, flags, data, res, std::move(amo_op)); } assert(req); if (!byteEnable.empty()) { diff --git a/src/cpu/simple/atomic.cc b/src/cpu/simple/atomic.cc index a873e6de7..9052cee2e 100644 --- a/src/cpu/simple/atomic.cc +++ b/src/cpu/simple/atomic.cc @@ -566,7 +566,7 @@ AtomicSimpleCPU::writeMem(uint8_t *data, unsigned size, Addr addr, Fault AtomicSimpleCPU::amoMem(Addr addr, uint8_t* data, unsigned size, - Request::Flags flags, AtomicOpFunctor *amo_op) + Request::Flags flags, AtomicOpFunctorPtr amo_op) { SimpleExecContext& t_info = *threadInfo[curThread]; SimpleThread* thread = t_info.thread; @@ -596,7 +596,7 @@ AtomicSimpleCPU::amoMem(Addr addr, uint8_t* data, unsigned size, req->taskId(taskId()); req->setVirt(0, addr, size, flags, dataMasterId(), - thread->pcState().instAddr(), amo_op); + thread->pcState().instAddr(), std::move(amo_op)); // translate to physical address Fault fault = thread->dtb->translateAtomic(req, thread->getTC(), diff --git a/src/cpu/simple/atomic.hh b/src/cpu/simple/atomic.hh index 69ac09e4c..121cecd65 100644 --- a/src/cpu/simple/atomic.hh +++ b/src/cpu/simple/atomic.hh @@ -227,7 +227,7 @@ class AtomicSimpleCPU : public BaseSimpleCPU override; Fault amoMem(Addr addr, uint8_t* data, unsigned size, - Request::Flags flags, AtomicOpFunctor *amo_op) override; + Request::Flags flags, AtomicOpFunctorPtr amo_op) override; void regProbePoints() override; diff --git a/src/cpu/simple/base.hh b/src/cpu/simple/base.hh index 5404e5df8..f8e534c85 100644 --- a/src/cpu/simple/base.hh +++ b/src/cpu/simple/base.hh @@ -162,12 +162,12 @@ class BaseSimpleCPU : public BaseCPU virtual Fault amoMem(Addr addr, uint8_t* data, unsigned size, Request::Flags flags, - AtomicOpFunctor *amo_op) + AtomicOpFunctorPtr amo_op) { panic("amoMem() is not implemented\n"); } virtual Fault initiateMemAMO(Addr addr, unsigned size, Request::Flags flags, - AtomicOpFunctor *amo_op) + AtomicOpFunctorPtr amo_op) { panic("initiateMemAMO() is not implemented\n"); } void countInst(); diff --git a/src/cpu/simple/exec_context.hh b/src/cpu/simple/exec_context.hh index de98d6efd..91f7ec526 100644 --- a/src/cpu/simple/exec_context.hh +++ b/src/cpu/simple/exec_context.hh @@ -463,16 +463,16 @@ class SimpleExecContext : public ExecContext { } Fault amoMem(Addr addr, uint8_t *data, unsigned int size, - Request::Flags flags, AtomicOpFunctor *amo_op) override + Request::Flags flags, AtomicOpFunctorPtr amo_op) override { - return cpu->amoMem(addr, data, size, flags, amo_op); + return cpu->amoMem(addr, data, size, flags, std::move(amo_op)); } Fault initiateMemAMO(Addr addr, unsigned int size, Request::Flags flags, - AtomicOpFunctor *amo_op) override + AtomicOpFunctorPtr amo_op) override { - return cpu->initiateMemAMO(addr, size, flags, amo_op); + return cpu->initiateMemAMO(addr, size, flags, std::move(amo_op)); } /** diff --git a/src/cpu/simple/timing.cc b/src/cpu/simple/timing.cc index 4aa008e33..d05eece27 100644 --- a/src/cpu/simple/timing.cc +++ b/src/cpu/simple/timing.cc @@ -564,7 +564,7 @@ TimingSimpleCPU::writeMem(uint8_t *data, unsigned size, Fault TimingSimpleCPU::initiateMemAMO(Addr addr, unsigned size, Request::Flags flags, - AtomicOpFunctor *amo_op) + AtomicOpFunctorPtr amo_op) { SimpleExecContext &t_info = *threadInfo[curThread]; SimpleThread* thread = t_info.thread; @@ -579,7 +579,8 @@ TimingSimpleCPU::initiateMemAMO(Addr addr, unsigned size, traceData->setMem(addr, size, flags); RequestPtr req = make_shared(asid, addr, size, flags, - dataMasterId(), pc, thread->contextId(), amo_op); + dataMasterId(), pc, thread->contextId(), + std::move(amo_op)); assert(req->hasAtomicOpFunctor()); diff --git a/src/cpu/simple/timing.hh b/src/cpu/simple/timing.hh index 53e0ed7e1..27faa177a 100644 --- a/src/cpu/simple/timing.hh +++ b/src/cpu/simple/timing.hh @@ -293,7 +293,7 @@ class TimingSimpleCPU : public BaseSimpleCPU override; Fault initiateMemAMO(Addr addr, unsigned size, Request::Flags flags, - AtomicOpFunctor *amo_op) override; + AtomicOpFunctorPtr amo_op) override; void fetch(); void sendFetch(const Fault &fault, diff --git a/src/mem/request.hh b/src/mem/request.hh index 324ae382e..50944932e 100644 --- a/src/mem/request.hh +++ b/src/mem/request.hh @@ -389,7 +389,7 @@ class Request InstSeqNum _reqInstSeqNum; /** A pointer to an atomic operation */ - AtomicOpFunctor *atomicOpFunctor; + AtomicOpFunctorPtr atomicOpFunctor; public: @@ -470,9 +470,9 @@ class Request Request(uint64_t asid, Addr vaddr, unsigned size, Flags flags, MasterID mid, Addr pc, ContextID cid, - AtomicOpFunctor *atomic_op) + AtomicOpFunctorPtr atomic_op) { - setVirt(asid, vaddr, size, flags, mid, pc, atomic_op); + setVirt(asid, vaddr, size, flags, mid, pc, std::move(atomic_op)); setContext(cid); } @@ -489,19 +489,13 @@ class Request translateDelta(other.translateDelta), accessDelta(other.accessDelta), depth(other.depth) { - if (other.atomicOpFunctor) - atomicOpFunctor = (other.atomicOpFunctor)->clone(); - else - atomicOpFunctor = nullptr; - } - ~Request() - { - if (hasAtomicOpFunctor()) { - delete atomicOpFunctor; - } + atomicOpFunctor.reset(other.atomicOpFunctor ? + other.atomicOpFunctor->clone() : nullptr); } + ~Request() {} + /** * Set up Context numbers. */ @@ -533,7 +527,7 @@ class Request */ void setVirt(uint64_t asid, Addr vaddr, unsigned size, Flags flags, - MasterID mid, Addr pc, AtomicOpFunctor *amo_op = nullptr) + MasterID mid, Addr pc, AtomicOpFunctorPtr amo_op = nullptr) { _asid = asid; _vaddr = vaddr; @@ -549,7 +543,7 @@ class Request depth = 0; accessDelta = 0; translateDelta = 0; - atomicOpFunctor = amo_op; + atomicOpFunctor = std::move(amo_op); } /** @@ -669,14 +663,14 @@ class Request bool hasAtomicOpFunctor() { - return atomicOpFunctor != NULL; + return (bool)atomicOpFunctor; } AtomicOpFunctor * getAtomicOpFunctor() { - assert(atomicOpFunctor != NULL); - return atomicOpFunctor; + assert(atomicOpFunctor); + return atomicOpFunctor.get(); } /** Accessor for flags. */ -- 2.30.2