cpu, mem: Changing AtomicOpFunctor* for unique_ptr<AtomicOpFunctor>

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<AtomicOpFunctor> 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 <andreas.sandberg@arm.com>
Maintainer: Andreas Sandberg <andreas.sandberg@arm.com>
Tested-by: kokoro <noreply+kokoro@google.com>

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 <class XC, class MemT>
 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 <class XC, class MemT>
 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<AtomicOpFunctor> 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<bool>& byteEnable = std::vector<bool>());
 
     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<class Impl>
 Fault
 BaseDynInst<Impl>::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<Impl>::initiateMemAMO(Addr addr, unsigned size,
     // memory
     return cpu->pushRequest(
             dynamic_cast<typename DynInstPtr::PtrType>(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<bool>& 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<bool>& byteEnable =
                           std::vector<bool>());
 
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<bool>& byteEnable =
                           std::vector<bool>())
 
     {
         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<bool> _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<Request>(_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<bool>& 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<class Impl>
 Fault
 LSQ<Impl>::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<bool>& byteEnable)
 {
     // This comming request can be either load, store or atomic.
@@ -717,7 +717,7 @@ LSQ<Impl>::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<Request>(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. */