mem: Add predecessor to SenderState base class

This patch adds a predecessor field to the SenderState base class to
make the process of linking them up more uniform, and enable a
traversal of the stack without knowing the specific type of the
subclasses.

There are a number of simplifications done as part of changing the
SenderState, particularly in the RubyTest.

12 files changed, 91 insertions, 86 deletions

diff --git a/src/mem/addr_mapper.cc b/src/mem/addr_mapper.cc
index 4ee834408..4aff9dcd8 100644
--- a/src/mem/addr_mapper.cc
+++ b/src/mem/addr_mapper.cc
@@ -123,10 +123,9 @@ AddrMapper::recvTimingReq(PacketPtr pkt)
     Addr orig_addr = pkt->getAddr();
     bool needsResponse = pkt->needsResponse();
     bool memInhibitAsserted = pkt->memInhibitAsserted();
-    Packet::SenderState* senderState = pkt->senderState;
 
     if (needsResponse && !memInhibitAsserted) {
-        pkt->senderState = new AddrMapperSenderState(senderState, orig_addr);
+        pkt->pushSenderState(new AddrMapperSenderState(orig_addr));
     }
 
     pkt->setAddr(remapAddr(orig_addr));
@@ -137,8 +136,7 @@ AddrMapper::recvTimingReq(PacketPtr pkt)
 
     // If not successful, restore the sender state
     if (!successful && needsResponse) {
-        delete pkt->senderState;
-        pkt->senderState = senderState;
+        delete pkt->popSenderState();
     }
 
     return successful;
@@ -158,7 +156,7 @@ AddrMapper::recvTimingResp(PacketPtr pkt)
     Addr remapped_addr = pkt->getAddr();
 
     // Restore the state and address
-    pkt->senderState = receivedState->origSenderState;
+    pkt->senderState = receivedState->predecessor;
     pkt->setAddr(receivedState->origAddr);
 
     // Attempt to send the packet
diff --git a/src/mem/addr_mapper.hh b/src/mem/addr_mapper.hh
index 887635999..6604096bd 100644
--- a/src/mem/addr_mapper.hh
+++ b/src/mem/addr_mapper.hh
@@ -87,23 +87,16 @@ class AddrMapper : public MemObject
       public:
 
         /**
-         * Construct a new sender state and remember the original one
-         * so that we can implement a stack.
+         * Construct a new sender state to remember the original address.
          *
-         * @param _origSenderState Sender state to remember
          * @param _origAddr Address before remapping
          */
-        AddrMapperSenderState(SenderState* _origSenderState,
-                              Addr _origAddr)
-            : origSenderState(_origSenderState), origAddr(_origAddr)
+        AddrMapperSenderState(Addr _origAddr) : origAddr(_origAddr)
         { }
 
         /** Destructor */
         ~AddrMapperSenderState() { }
 
-        /** Pointer to old sender state of packet */
-        SenderState* origSenderState;
-
         /** The original address the packet was destined for */
         Addr origAddr;
 
diff --git a/src/mem/bridge.cc b/src/mem/bridge.cc
index bece5e6a1..bfe7e795c 100644
--- a/src/mem/bridge.cc
+++ b/src/mem/bridge.cc
@@ -201,8 +201,7 @@ Bridge::BridgeMasterPort::schedTimingReq(PacketPtr pkt, Tick when)
     if (!pkt->memInhibitAsserted() && pkt->needsResponse()) {
         // Update the sender state so we can deal with the response
         // appropriately
-        RequestState *req_state = new RequestState(pkt);
-        pkt->senderState = req_state;
+        pkt->pushSenderState(new RequestState(pkt->getSrc()));
     }
 
     // If we're about to put this packet at the head of the queue, we
@@ -225,11 +224,10 @@ Bridge::BridgeSlavePort::schedTimingResp(PacketPtr pkt, Tick when)
     // This is a response for a request we forwarded earlier.  The
     // corresponding request state should be stored in the packet's
     // senderState field.
-    RequestState *req_state = dynamic_cast<RequestState*>(pkt->senderState);
+    RequestState *req_state =
+        dynamic_cast<RequestState*>(pkt->popSenderState());
     assert(req_state != NULL);
-    // set up new packet dest & senderState based on values saved
-    // from original request
-    req_state->fixResponse(pkt);
+    pkt->setDest(req_state->origSrc);
     delete req_state;
 
     // the bridge assumes that at least one bus has set the
diff --git a/src/mem/bridge.hh b/src/mem/bridge.hh
index 6855d2722..2e594a30a 100644
--- a/src/mem/bridge.hh
+++ b/src/mem/bridge.hh
@@ -84,20 +84,11 @@ class Bridge : public MemObject
 
       public:
 
-        Packet::SenderState *origSenderState;
         PortID origSrc;
 
-        RequestState(PacketPtr _pkt)
-            : origSenderState(_pkt->senderState),
-              origSrc(_pkt->getSrc())
+        RequestState(PortID orig_src) : origSrc(orig_src)
         { }
 
-        void fixResponse(PacketPtr pkt)
-        {
-            assert(pkt->senderState == this);
-            pkt->setDest(origSrc);
-            pkt->senderState = origSenderState;
-        }
     };
 
     /**
diff --git a/src/mem/cache/cache_impl.hh b/src/mem/cache/cache_impl.hh
index a5f1b4844..d2c9f900e 100644
--- a/src/mem/cache/cache_impl.hh
+++ b/src/mem/cache/cache_impl.hh
@@ -348,24 +348,12 @@ Cache<TagStore>::access(PacketPtr pkt, BlkType *&blk,
 
 class ForwardResponseRecord : public Packet::SenderState
 {
-    Packet::SenderState *prevSenderState;
-    PortID prevSrc;
-#ifndef NDEBUG
-    BaseCache *cache;
-#endif
   public:
-    ForwardResponseRecord(Packet *pkt, BaseCache *_cache)
-        : prevSenderState(pkt->senderState), prevSrc(pkt->getSrc())
-#ifndef NDEBUG
-          , cache(_cache)
-#endif
+
+    PortID prevSrc;
+
+    ForwardResponseRecord(PortID prev_src) : prevSrc(prev_src)
     {}
-    void restore(Packet *pkt, BaseCache *_cache)
-    {
-        assert(_cache == cache);
-        pkt->senderState = prevSenderState;
-        pkt->setDest(prevSrc);
-    }
 };
 
 
@@ -389,7 +377,7 @@ Cache<TagStore>::timingAccess(PacketPtr pkt)
     if (pkt->isResponse()) {
         // must be cache-to-cache response from upper to lower level
         ForwardResponseRecord *rec =
-            dynamic_cast<ForwardResponseRecord *>(pkt->senderState);
+            dynamic_cast<ForwardResponseRecord *>(pkt->popSenderState());
         assert(!system->bypassCaches());
 
         if (rec == NULL) {
@@ -402,7 +390,7 @@ Cache<TagStore>::timingAccess(PacketPtr pkt)
             return true;
         }
 
-        rec->restore(pkt, this);
+        pkt->setDest(rec->prevSrc);
         delete rec;
         memSidePort->schedTimingSnoopResp(pkt, time);
         return true;
@@ -1293,14 +1281,14 @@ Cache<TagStore>::handleSnoop(PacketPtr pkt, BlkType *blk,
         if (is_timing) {
             Packet snoopPkt(pkt, true);  // clear flags
             snoopPkt.setExpressSnoop();
-            snoopPkt.senderState = new ForwardResponseRecord(pkt, this);
+            snoopPkt.pushSenderState(new ForwardResponseRecord(pkt->getSrc()));
             cpuSidePort->sendTimingSnoopReq(&snoopPkt);
             if (snoopPkt.memInhibitAsserted()) {
                 // cache-to-cache response from some upper cache
                 assert(!alreadyResponded);
                 pkt->assertMemInhibit();
             } else {
-                delete snoopPkt.senderState;
+                delete snoopPkt.popSenderState();
             }
             if (snoopPkt.sharedAsserted()) {
                 pkt->assertShared();
diff --git a/src/mem/comm_monitor.cc b/src/mem/comm_monitor.cc
index 51e95b36b..a6c08e3b2 100644
--- a/src/mem/comm_monitor.cc
+++ b/src/mem/comm_monitor.cc
@@ -167,15 +167,13 @@ CommMonitor::recvTimingReq(PacketPtr pkt)
     Addr addr = pkt->getAddr();
     bool needsResponse = pkt->needsResponse();
     bool memInhibitAsserted = pkt->memInhibitAsserted();
-    Packet::SenderState* senderState = pkt->senderState;
 
     // If a cache miss is served by a cache, a monitor near the memory
     // would see a request which needs a response, but this response
     // would be inhibited and not come back from the memory. Therefore
     // we additionally have to check the inhibit flag.
     if (needsResponse && !memInhibitAsserted && !stats.disableLatencyHists) {
-        pkt->senderState = new CommMonitorSenderState(senderState,
-                                                      curTick());
+        pkt->pushSenderState(new CommMonitorSenderState(curTick()));
     }
 
     // Attempt to send the packet (always succeeds for inhibited
@@ -184,8 +182,7 @@ CommMonitor::recvTimingReq(PacketPtr pkt)
 
     // If not successful, restore the sender state
     if (!successful && needsResponse && !stats.disableLatencyHists) {
-        delete pkt->senderState;
-        pkt->senderState = senderState;
+        delete pkt->popSenderState();
     }
 
     if (successful && traceStream != NULL) {
@@ -306,7 +303,7 @@ CommMonitor::recvTimingResp(PacketPtr pkt)
             panic("Monitor got a response without monitor sender state\n");
 
         // Restore the sate
-        pkt->senderState = commReceivedState->origSenderState;
+        pkt->senderState = commReceivedState->predecessor;
     }
 
     // Attempt to send the packet
diff --git a/src/mem/comm_monitor.hh b/src/mem/comm_monitor.hh
index 271ae5fff..c3cb0d352 100644
--- a/src/mem/comm_monitor.hh
+++ b/src/mem/comm_monitor.hh
@@ -107,23 +107,18 @@ class CommMonitor : public MemObject
       public:
 
         /**
-         * Construct a new sender state and remember the original one
-         * so that we can implement a stack.
+         * Construct a new sender state and store the time so we can
+         * calculate round-trip latency.
          *
-         * @param _origSenderState Sender state to remember
          * @param _transmitTime Time of packet transmission
          */
-        CommMonitorSenderState(SenderState* _origSenderState,
-                               Tick _transmitTime)
-            : origSenderState(_origSenderState), transmitTime(_transmitTime)
+        CommMonitorSenderState(Tick _transmitTime)
+            : transmitTime(_transmitTime)
         { }
 
         /** Destructor */
         ~CommMonitorSenderState() { }
 
-        /** Pointer to old sender state of packet */
-        SenderState* origSenderState;
-
         /** Tick when request is transmitted */
         Tick transmitTime;
 
diff --git a/src/mem/packet.cc b/src/mem/packet.cc
index dc5ff4362..cea65cea0 100644
--- a/src/mem/packet.cc
+++ b/src/mem/packet.cc
@@ -316,6 +316,24 @@ Packet::checkFunctional(Printable *obj, Addr addr, int size, uint8_t *data)
 }
 
 void
+Packet::pushSenderState(Packet::SenderState *sender_state)
+{
+    assert(sender_state != NULL);
+    sender_state->predecessor = senderState;
+    senderState = sender_state;
+}
+
+Packet::SenderState *
+Packet::popSenderState()
+{
+    assert(senderState != NULL);
+    SenderState *sender_state = senderState;
+    senderState = sender_state->predecessor;
+    sender_state->predecessor = NULL;
+    return sender_state;
+}
+
+void
 Packet::print(ostream &o, const int verbosity, const string &prefix) const
 {
     ccprintf(o, "%s[%x:%x] %s\n", prefix,
diff --git a/src/mem/packet.hh b/src/mem/packet.hh
index fbcf185cc..6da1fe97d 100644
--- a/src/mem/packet.hh
+++ b/src/mem/packet.hh
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012 ARM Limited
+ * Copyright (c) 2012-2013 ARM Limited
  * All rights reserved
  *
  * The license below extends only to copyright in the software and shall
@@ -340,15 +340,25 @@ class Packet : public Printable
 
     /**
      * A virtual base opaque structure used to hold state associated
-     * with the packet but specific to the sending device (e.g., an
-     * MSHR).  A pointer to this state is returned in the packet's
-     * response so that the sender can quickly look up the state
-     * needed to process it.  A specific subclass would be derived
-     * from this to carry state specific to a particular sending
-     * device.
+     * with the packet (e.g., an MSHR), specific to a MemObject that
+     * sees the packet. A pointer to this state is returned in the
+     * packet's response so that the MemObject in question can quickly
+     * look up the state needed to process it. A specific subclass
+     * would be derived from this to carry state specific to a
+     * particular sending device.
+     *
+     * As multiple MemObjects may add their SenderState throughout the
+     * memory system, the SenderStates create a stack, where a
+     * MemObject can add a new Senderstate, as long as the
+     * predecessing SenderState is restored when the response comes
+     * back. For this reason, the predecessor should always be
+     * populated with the current SenderState of a packet before
+     * modifying the senderState field in the request packet.
      */
     struct SenderState
     {
+        SenderState* predecessor;
+        SenderState() : predecessor(NULL) {}
         virtual ~SenderState() {}
     };
 
@@ -418,12 +428,32 @@ class Packet : public Printable
      * This packet's sender state.  Devices should use dynamic_cast<>
      * to cast to the state appropriate to the sender.  The intent of
      * this variable is to allow a device to attach extra information
-     * to a request.  A response packet must return the sender state
+     * to a request. A response packet must return the sender state
      * that was attached to the original request (even if a new packet
      * is created).
      */
     SenderState *senderState;
 
+    /**
+     * Push a new sender state to the packet and make the current
+     * sender state the predecessor of the new one. This should be
+     * prefered over direct manipulation of the senderState member
+     * variable.
+     *
+     * @param sender_state SenderState to push at the top of the stack
+     */
+    void pushSenderState(SenderState *sender_state);
+
+    /**
+     * Pop the top of the state stack and return a pointer to it. This
+     * assumes the current sender state is not NULL. This should be
+     * preferred over direct manipulation of the senderState member
+     * variable.
+     *
+     * @return The current top of the stack
+     */
+    SenderState *popSenderState();
+
     /// Return the string name of the cmd field (for debugging and
     /// tracing).
     const std::string &cmdString() const { return cmd.toString(); }
diff --git a/src/mem/ruby/system/RubyPort.cc b/src/mem/ruby/system/RubyPort.cc
index 2f1cc622d..5e9e8cdd4 100644
--- a/src/mem/ruby/system/RubyPort.cc
+++ b/src/mem/ruby/system/RubyPort.cc
@@ -151,12 +151,9 @@ RubyPort::PioPort::recvTimingResp(PacketPtr pkt)
 
     // First we must retrieve the request port from the sender State
     RubyPort::SenderState *senderState =
-      safe_cast<RubyPort::SenderState *>(pkt->senderState);
+        safe_cast<RubyPort::SenderState *>(pkt->popSenderState());
     M5Port *port = senderState->port;
     assert(port != NULL);
-
-    // pop the sender state from the packet
-    pkt->senderState = senderState->saved;
     delete senderState;
 
     port->sendTimingResp(pkt);
@@ -187,7 +184,7 @@ RubyPort::M5Port::recvTimingReq(PacketPtr pkt)
 
     // Save the port in the sender state object to be used later to
     // route the response
-    pkt->senderState = new SenderState(this, pkt->senderState);
+    pkt->pushSenderState(new SenderState(this));
 
     // Check for pio requests and directly send them to the dedicated
     // pio port.
@@ -230,7 +227,7 @@ RubyPort::M5Port::recvTimingReq(PacketPtr pkt)
             pkt->getAddr(), RequestStatus_to_string(requestStatus));
 
     SenderState* senderState = safe_cast<SenderState*>(pkt->senderState);
-    pkt->senderState = senderState->saved;
+    pkt->senderState = senderState->predecessor;
     delete senderState;
     return false;
 }
@@ -305,7 +302,7 @@ RubyPort::ruby_hit_callback(PacketPtr pkt)
     assert(port != NULL);
 
     // pop the sender state from the packet
-    pkt->senderState = senderState->saved;
+    pkt->senderState = senderState->predecessor;
     delete senderState;
 
     port->hitCallback(pkt);
diff --git a/src/mem/ruby/system/RubyPort.hh b/src/mem/ruby/system/RubyPort.hh
index cec356edb..3c61eb522 100644
--- a/src/mem/ruby/system/RubyPort.hh
+++ b/src/mem/ruby/system/RubyPort.hh
@@ -113,10 +113,8 @@ class RubyPort : public MemObject
     struct SenderState : public Packet::SenderState
     {
         M5Port* port;
-        Packet::SenderState *saved;
 
-        SenderState(M5Port* _port, Packet::SenderState *sender_state = NULL)
-            : port(_port), saved(sender_state)
+        SenderState(M5Port* _port) : port(_port)
         {}
     };
 
diff --git a/src/mem/ruby/system/Sequencer.cc b/src/mem/ruby/system/Sequencer.cc
index 1cdd6d806..f00f8407a 100644
--- a/src/mem/ruby/system/Sequencer.cc
+++ b/src/mem/ruby/system/Sequencer.cc
@@ -530,11 +530,13 @@ Sequencer::hitCallback(SequencerRequest* srequest,
     // Note: RubyPort will access it's sender state before the
     // RubyTester.
     if (m_usingRubyTester) {
-        RubyPort::SenderState *requestSenderState =
+        RubyPort::SenderState *reqSenderState =
             safe_cast<RubyPort::SenderState*>(pkt->senderState);
+        // @todo This is a dangerous assumption on nothing else
+        // modifying the senderState
         RubyTester::SenderState* testerSenderState =
-            safe_cast<RubyTester::SenderState*>(requestSenderState->saved);
-        testerSenderState->subBlock->mergeFrom(data);
+            safe_cast<RubyTester::SenderState*>(reqSenderState->predecessor);
+        testerSenderState->subBlock.mergeFrom(data);
     }
 
     delete srequest;