mem: Add PacketInfo to be used for packet probe points

This patch fixes a use-after-delete issue in the packet probe points
by adding a PacketInfo struct to retain the key fields before passing
the packet onwards. We want to probe the packet after it is
successfully sent, but by that time the fields may be modified, and
the packet may even be deleted.

Amazingly enough the issue has gone undetected for months, and only
recently popped up in our regressions.

diff --git a/src/mem/comm_monitor.cc b/src/mem/comm_monitor.cc
index beb7fe32c08eb3c5f681eeb273f2d07edbc638e6..85d7be2f4c9b6dab8fb4b7a1914baceb35840000 100644 (file)
--- a/src/mem/comm_monitor.cc
+++ b/src/mem/comm_monitor.cc
@@ -115,11 +115,13 @@ CommMonitor::recvFunctionalSnoop(PacketPtr pkt)
 Tick
 CommMonitor::recvAtomic(PacketPtr pkt)
 {
-    ppPktReq->notify(pkt);
+    ProbePoints::PacketInfo req_pkt_info(pkt);
+    ppPktReq->notify(req_pkt_info);
 
     const Tick delay(masterPort.sendAtomic(pkt));
     assert(pkt->isResponse());
-    ppPktResp->notify(pkt);
+    ProbePoints::PacketInfo resp_pkt_info(pkt);
+    ppPktResp->notify(resp_pkt_info);
     return delay;
 }
 
@@ -137,11 +139,10 @@ CommMonitor::recvTimingReq(PacketPtr pkt)
 
     // Store relevant fields of packet, because packet may be modified
     // or even deleted when sendTiming() is called.
+    const ProbePoints::PacketInfo pkt_info(pkt);
+
     const bool is_read = pkt->isRead();
     const bool is_write = pkt->isWrite();
-    const MemCmd cmd = pkt->cmd;
-    const unsigned size = pkt->getSize();
-    const Addr addr = pkt->getAddr();
     const bool expects_response(
         pkt->needsResponse() && !pkt->memInhibitAsserted());
 
@@ -163,14 +164,7 @@ CommMonitor::recvTimingReq(PacketPtr pkt)
     }
 
     if (successful) {
-        // The receiver might already have modified the packet. We
-        // want to give the probe access to the original packet, which
-        // means we need to fake the original packet by temporarily
-        // restoring the command.
-        const MemCmd response_cmd(pkt->cmd);
-        pkt->cmd = cmd;
-        ppPktReq->notify(pkt);
-        pkt->cmd = response_cmd;
+        ppPktReq->notify(pkt_info);
     }
 
     if (successful && is_read) {
@@ -183,12 +177,12 @@ CommMonitor::recvTimingReq(PacketPtr pkt)
 
         // Get sample of burst length
         if (!stats.disableBurstLengthHists) {
-            stats.readBurstLengthHist.sample(size);
+            stats.readBurstLengthHist.sample(pkt_info.size);
         }
 
         // Sample the masked address
         if (!stats.disableAddrDists) {
-            stats.readAddrDist.sample(addr & readAddrMask);
+            stats.readAddrDist.sample(pkt_info.addr & readAddrMask);
         }
 
         // If it needs a response increment number of outstanding read
@@ -219,18 +213,18 @@ CommMonitor::recvTimingReq(PacketPtr pkt)
         }
 
         if (!stats.disableBurstLengthHists) {
-            stats.writeBurstLengthHist.sample(size);
+            stats.writeBurstLengthHist.sample(pkt_info.size);
         }
 
         // Update the bandwidth stats on the request
         if (!stats.disableBandwidthHists) {
-            stats.writtenBytes += size;
-            stats.totalWrittenBytes += size;
+            stats.writtenBytes += pkt_info.size;
+            stats.totalWrittenBytes += pkt_info.size;
         }
 
         // Sample the masked write address
         if (!stats.disableAddrDists) {
-            stats.writeAddrDist.sample(addr & writeAddrMask);
+            stats.writeAddrDist.sample(pkt_info.addr & writeAddrMask);
         }
 
         if (!stats.disableOutstandingHists && expects_response) {
@@ -265,9 +259,10 @@ CommMonitor::recvTimingResp(PacketPtr pkt)
 
     // Store relevant fields of packet, because packet may be modified
     // or even deleted when sendTiming() is called.
+    const ProbePoints::PacketInfo pkt_info(pkt);
+
     bool is_read = pkt->isRead();
     bool is_write = pkt->isWrite();
-    unsigned size = pkt->getSize();
     Tick latency = 0;
     CommMonitorSenderState* received_state =
         dynamic_cast<CommMonitorSenderState*>(pkt->senderState);
@@ -299,8 +294,7 @@ CommMonitor::recvTimingResp(PacketPtr pkt)
     }
 
     if (successful) {
-        assert(pkt->isResponse());
-        ppPktResp->notify(pkt);
+        ppPktResp->notify(pkt_info);
     }
 
     if (successful && is_read) {
@@ -317,8 +311,8 @@ CommMonitor::recvTimingResp(PacketPtr pkt)
 
         // Update the bandwidth stats based on responses for reads
         if (!stats.disableBandwidthHists) {
-            stats.readBytes += size;
-            stats.totalReadBytes += size;
+            stats.readBytes += pkt_info.size;
+            stats.totalReadBytes += pkt_info.size;
         }
 
     } else if (successful && is_write) {

diff --git a/src/mem/probes/base.hh b/src/mem/probes/base.hh
index 7a0a7ea052d39bd61cbfcf0b8037843f1909254f..ccb5ca749b8c05fe6eb31cffc9eb294656d66577 100644 (file)
--- a/src/mem/probes/base.hh
+++ b/src/mem/probes/base.hh
@@ -43,8 +43,7 @@
 #include <memory>
 #include <vector>
 
-#include "mem/packet.hh"
-#include "sim/probe/probe.hh"
+#include "sim/probe/mem.hh"
 #include "sim/sim_object.hh"
 
 struct BaseMemProbeParams;
@@ -72,10 +71,10 @@ class BaseMemProbe : public SimObject
     /**
      * Callback to analyse intercepted Packets.
      */
-    virtual void handleRequest(const PacketPtr &pkt) = 0;
+    virtual void handleRequest(const ProbePoints::PacketInfo &pkt_info) = 0;
 
   private:
-    class PacketListener : public ProbeListenerArgBase<PacketPtr>
+    class PacketListener : public ProbeListenerArgBase<ProbePoints::PacketInfo>
     {
       public:
         PacketListener(BaseMemProbe &_parent,
@@ -83,8 +82,8 @@ class BaseMemProbe : public SimObject
             : ProbeListenerArgBase(pm, name),
               parent(_parent) {}
 
-        void notify(const PacketPtr &pkt) M5_ATTR_OVERRIDE {
-            parent.handleRequest(pkt);
+        void notify(const ProbePoints::PacketInfo &pkt_info) M5_ATTR_OVERRIDE {
+            parent.handleRequest(pkt_info);
         }
 
       protected:

diff --git a/src/mem/probes/mem_trace.cc b/src/mem/probes/mem_trace.cc
index 6bf6f7f7761c51d73af08e804e6871ba8dab0f60..121c6de48e01e8dc07135cb7078128dc53b19418 100644 (file)
--- a/src/mem/probes/mem_trace.cc
+++ b/src/mem/probes/mem_trace.cc
@@ -93,15 +93,15 @@ MemTraceProbe::closeStreams()
 }
 
 void
-MemTraceProbe::handleRequest(const PacketPtr &pkt)
+MemTraceProbe::handleRequest(const ProbePoints::PacketInfo &pkt_info)
 {
     ProtoMessage::Packet pkt_msg;
 
     pkt_msg.set_tick(curTick());
-    pkt_msg.set_cmd(pkt->cmdToIndex());
-    pkt_msg.set_flags(pkt->req->getFlags());
-    pkt_msg.set_addr(pkt->getAddr());
-    pkt_msg.set_size(pkt->getSize());
+    pkt_msg.set_cmd(pkt_info.cmd.toInt());
+    pkt_msg.set_flags(pkt_info.flags);
+    pkt_msg.set_addr(pkt_info.addr);
+    pkt_msg.set_size(pkt_info.size);
 
     traceStream->write(pkt_msg);
 }

diff --git a/src/mem/probes/mem_trace.hh b/src/mem/probes/mem_trace.hh
index 677b8ae32d9b6a1791ad8b8ab897e0f5175aeb50..51f272812c1eda558b905b7c270f72eb149f3772 100644 (file)
--- a/src/mem/probes/mem_trace.hh
+++ b/src/mem/probes/mem_trace.hh
@@ -52,7 +52,8 @@ class MemTraceProbe : public BaseMemProbe
     MemTraceProbe(MemTraceProbeParams *params);
 
   protected:
-    void handleRequest(const PacketPtr &pkt) M5_ATTR_OVERRIDE;
+    void handleRequest(const ProbePoints::PacketInfo &pkt_info) \
+        M5_ATTR_OVERRIDE;
 
     /**
      * Callback to flush and close all open output streams on exit. If

diff --git a/src/mem/probes/stack_dist.cc b/src/mem/probes/stack_dist.cc
index c742cae7b52e557e143d2624b2b1d19eb8402094..a447f49e5ed8f8c519849a9b12e9fcfc19607f3b 100644 (file)
--- a/src/mem/probes/stack_dist.cc
+++ b/src/mem/probes/stack_dist.cc
@@ -94,15 +94,15 @@ StackDistProbe::regStats()
 }
 
 void
-StackDistProbe::handleRequest(const PacketPtr &pkt)
+StackDistProbe::handleRequest(const ProbePoints::PacketInfo &pkt_info)
 {
     // only capturing read and write requests (which allocate in the
     // cache)
-    if (!pkt->isRead() && !pkt->isWrite())
+    if (!pkt_info.cmd.isRead() && !pkt_info.cmd.isWrite())
         return;
 
     // Align the address to a cache line size
-    const Addr aligned_addr(roundDown(pkt->getAddr(), lineSize));
+    const Addr aligned_addr(roundDown(pkt_info.addr, lineSize));
 
     // Calculate the stack distance
     const uint64_t sd(calc.calcStackDistAndUpdate(aligned_addr).first);
@@ -113,7 +113,7 @@ StackDistProbe::handleRequest(const PacketPtr &pkt)
 
     // Sample the stack distance of the address in linear bins
     if (!disableLinearHists) {
-        if (pkt->isRead())
+        if (pkt_info.cmd.isRead())
             readLinearHist.sample(sd);
         else
             writeLinearHist.sample(sd);
@@ -123,7 +123,7 @@ StackDistProbe::handleRequest(const PacketPtr &pkt)
         int sd_lg2 = sd == 0 ? 1 : floorLog2(sd);
 
         // Sample the stack distance of the address in log bins
-        if (pkt->isRead())
+        if (pkt_info.cmd.isRead())
             readLogHist.sample(sd_lg2);
         else
             writeLogHist.sample(sd_lg2);

diff --git a/src/mem/probes/stack_dist.hh b/src/mem/probes/stack_dist.hh
index 2108008946777d8ddbea87efdc7fd47a11e32d76..8374672daae91bc6603d7d8f289499546e1ff3b0 100644 (file)
--- a/src/mem/probes/stack_dist.hh
+++ b/src/mem/probes/stack_dist.hh
@@ -55,7 +55,8 @@ class StackDistProbe : public BaseMemProbe
     void regStats() M5_ATTR_OVERRIDE;
 
   protected:
-    void handleRequest(const PacketPtr &pkt) M5_ATTR_OVERRIDE;
+    void handleRequest(const ProbePoints::PacketInfo &pkt_info) \
+        M5_ATTR_OVERRIDE;
 
   protected:
     // Cache line size to simulate

diff --git a/src/sim/probe/mem.hh b/src/sim/probe/mem.hh
index 506287140fc60570cc36f078f0640ee1d237dea0..2d4b9aeec5ba64cd29e14a4dced59018136021d8 100644 (file)
--- a/src/sim/probe/mem.hh
+++ b/src/sim/probe/mem.hh
@@ -46,6 +46,24 @@
 
 namespace ProbePoints {
 
+/**
+ * A struct to hold on to the essential fields from a packet, so that
+ * the packet and underlying request can be safely passed on, and
+ * consequently modified or even deleted.
+ */
+struct PacketInfo {
+    MemCmd cmd;
+    Addr addr;
+    uint32_t size;
+    Request::FlagsType flags;
+
+    explicit PacketInfo(const PacketPtr& pkt) :
+        cmd(pkt->cmd),
+        addr(pkt->getAddr()),
+        size(pkt->getSize()),
+        flags(pkt->req->getFlags()) { }
+};
+
 /**
  * Packet probe point
  *
@@ -79,7 +97,7 @@ namespace ProbePoints {
  * </ul>
  *
  */
-typedef ProbePointArg< ::PacketPtr> Packet;
+typedef ProbePointArg<PacketInfo> Packet;
 typedef std::unique_ptr<Packet> PacketUPtr;
 
 }