changeset bd894d2bdd7c in /z/repo/gem5
details: http://repo.gem5.org/gem5?cmd=changeset;node=bd894d2bdd7c
description:
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.
diffstat:
src/mem/comm_monitor.cc | 42 ++++++++++++++++++------------------------
src/mem/probes/base.hh | 11 +++++------
src/mem/probes/mem_trace.cc | 10 +++++-----
src/mem/probes/mem_trace.hh | 3 ++-
src/mem/probes/stack_dist.cc | 10 +++++-----
src/mem/probes/stack_dist.hh | 3 ++-
src/sim/probe/mem.hh | 20 +++++++++++++++++++-
7 files changed, 56 insertions(+), 43 deletions(-)
diffs (284 lines):
diff -r a611a23c8cc2 -r bd894d2bdd7c src/mem/comm_monitor.cc
--- a/src/mem/comm_monitor.cc Fri Sep 25 07:27:03 2015 -0400
+++ b/src/mem/comm_monitor.cc Fri Sep 25 13:25:34 2015 -0400
@@ -115,11 +115,13 @@
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 @@
// 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 @@
}
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 @@
// 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 @@
}
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 @@
// 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 @@
}
if (successful) {
- assert(pkt->isResponse());
- ppPktResp->notify(pkt);
+ ppPktResp->notify(pkt_info);
}
if (successful && is_read) {
@@ -317,8 +311,8 @@
// 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 -r a611a23c8cc2 -r bd894d2bdd7c src/mem/probes/base.hh
--- a/src/mem/probes/base.hh Fri Sep 25 07:27:03 2015 -0400
+++ b/src/mem/probes/base.hh Fri Sep 25 13:25:34 2015 -0400
@@ -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 @@
/**
* 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 @@
: 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 -r a611a23c8cc2 -r bd894d2bdd7c src/mem/probes/mem_trace.cc
--- a/src/mem/probes/mem_trace.cc Fri Sep 25 07:27:03 2015 -0400
+++ b/src/mem/probes/mem_trace.cc Fri Sep 25 13:25:34 2015 -0400
@@ -93,15 +93,15 @@
}
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 -r a611a23c8cc2 -r bd894d2bdd7c src/mem/probes/mem_trace.hh
--- a/src/mem/probes/mem_trace.hh Fri Sep 25 07:27:03 2015 -0400
+++ b/src/mem/probes/mem_trace.hh Fri Sep 25 13:25:34 2015 -0400
@@ -52,7 +52,8 @@
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 -r a611a23c8cc2 -r bd894d2bdd7c src/mem/probes/stack_dist.cc
--- a/src/mem/probes/stack_dist.cc Fri Sep 25 07:27:03 2015 -0400
+++ b/src/mem/probes/stack_dist.cc Fri Sep 25 13:25:34 2015 -0400
@@ -94,15 +94,15 @@
}
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 @@
// 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 @@
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 -r a611a23c8cc2 -r bd894d2bdd7c src/mem/probes/stack_dist.hh
--- a/src/mem/probes/stack_dist.hh Fri Sep 25 07:27:03 2015 -0400
+++ b/src/mem/probes/stack_dist.hh Fri Sep 25 13:25:34 2015 -0400
@@ -55,7 +55,8 @@
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 -r a611a23c8cc2 -r bd894d2bdd7c src/sim/probe/mem.hh
--- a/src/sim/probe/mem.hh Fri Sep 25 07:27:03 2015 -0400
+++ b/src/sim/probe/mem.hh Fri Sep 25 13:25:34 2015 -0400
@@ -47,6 +47,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
*
* This probe point provides a unified interface for components that
@@ -79,7 +97,7 @@
* </ul>
*
*/
-typedef ProbePointArg< ::PacketPtr> Packet;
+typedef ProbePointArg<PacketInfo> Packet;
typedef std::unique_ptr<Packet> PacketUPtr;
}
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
