From: Andreas Hansson Date: Thu, 17 Dec 2015 22:07:11 +0000 (-0500) Subject: mem: Fix memory allocation bug in deferred snoop handling X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=97887eb6dc9e548c9b5719727fd4783ef157917c;p=gem5.git mem: Fix memory allocation bug in deferred snoop handling This patch fixes a corner case in the deferred snoop handling, where requests ended up being used by multiple packets with different lifetimes, and inadvertently got deleted while they were still in use. --- diff --git a/src/mem/cache/cache.cc b/src/mem/cache/cache.cc index 29919ccdf..67e889453 100644 --- a/src/mem/cache/cache.cc +++ b/src/mem/cache/cache.cc @@ -2005,15 +2005,9 @@ Cache::handleSnoop(PacketPtr pkt, CacheBlk *blk, bool is_timing, } if (!respond && is_timing && is_deferred) { - // if it's a deferred timing snoop then we've made a copy of - // both the request and the packet, and so if we're not using - // those copies to respond and delete them here - DPRINTF(Cache, "Deleting pkt %p and request %p for cmd %s addr: %p\n", - pkt, pkt->req, pkt->cmdString(), pkt->getAddr()); - - // the packets needs a response (just not from us), so we also - // need to delete the request and not rely on the packet - // destructor + // if it's a deferred timing snoop to which we are not + // responding, then we've made a copy of both the request and + // the packet, delete them here assert(pkt->needsResponse()); delete pkt->req; delete pkt; diff --git a/src/mem/cache/mshr.cc b/src/mem/cache/mshr.cc index b58c256cd..1a97d5594 100644 --- a/src/mem/cache/mshr.cc +++ b/src/mem/cache/mshr.cc @@ -364,36 +364,37 @@ MSHR::handleSnoop(PacketPtr pkt, Counter _order) if (isPendingDirty() || pkt->isInvalidate()) { // We need to save and replay the packet in two cases: // 1. We're awaiting an exclusive copy, so ownership is pending, - // and we need to respond after we receive data. + // and we need to deal with the snoop after we receive data. // 2. It's an invalidation (e.g., UpgradeReq), and we need // to forward the snoop up the hierarchy after the current // transaction completes. - - // Actual target device (typ. a memory) will delete the - // packet on reception, so we need to save a copy here. - // Clear flags and also allocate new data as the original - // packet data storage may have been deleted by the time we - // get to send this packet. - PacketPtr cp_pkt = nullptr; + // Start by determining if we will eventually respond or not, + // matching the conditions checked in Cache::handleSnoop + bool will_respond = isPendingDirty() && pkt->needsResponse() && + pkt->cmd != MemCmd::InvalidateReq; + + // The packet we are snooping may be deleted by the time we + // actually process the target, and we consequently need to + // save a copy here. Clear flags and also allocate new data as + // the original packet data storage may have been deleted by + // the time we get to process this packet. In the cases where + // we are not responding after handling the snoop we also need + // to create a copy of the request to be on the safe side. In + // the latter case the cache is responsible for deleting both + // the packet and the request as part of handling the deferred + // snoop. + PacketPtr cp_pkt = will_respond ? new Packet(pkt, true, true) : + new Packet(new Request(*pkt->req), pkt->cmd); if (isPendingDirty()) { - // Case 1: The new packet will need to get the response from the + // The new packet will need to get the response from the // MSHR already queued up here - cp_pkt = new Packet(pkt, true, true); pkt->assertMemInhibit(); // in the case of an uncacheable request there is no need // to set the exclusive flag, but since the recipient does // not care there is no harm in doing so pkt->setSupplyExclusive(); - } else { - // Case 2: We only need to buffer the packet for information - // purposes; the original request can proceed without waiting - // => Create a copy of the request, as that may get deallocated as - // well - cp_pkt = new Packet(new Request(*pkt->req), pkt->cmd); - DPRINTF(Cache, "Copying packet %p -> %p and request %p -> %p\n", - pkt, cp_pkt, pkt->req, cp_pkt->req); } targets.add(cp_pkt, curTick(), _order, Target::FromSnoop, downstreamPending && targets.needsExclusive);