mem: Cleanup Packet::checkFunctional and hasData usage

This patch cleans up the use of hasData and checkFunctional in the
packet. The hasData function is unfortunately suggesting that it
checks if the packet has a valid data pointer, when it does in fact
only check if the specific packet type is specified to have a data
payload. The confusion led to a bug in checkFunctional. The latter
function is also tidied up to avoid name overloading.

3 files changed, 48 insertions, 23 deletions

diff --git a/src/mem/cache/cache_impl.hh b/src/mem/cache/cache_impl.hh
index 8e8079d58..296f31ebd 100644
--- a/src/mem/cache/cache_impl.hh
+++ b/src/mem/cache/cache_impl.hh
@@ -1481,6 +1481,10 @@ Cache<TagStore>::handleFill(PacketPtr pkt, BlkType *blk,
     if (blk == NULL) {
         // better have read new data...
         assert(pkt->hasData());
+
+        // only read reaponses have data
+        assert(pkt->isRead());
+
         // need to do a replacement
         blk = allocateBlock(addr, is_secure, writebacks);
         if (blk == NULL) {
@@ -1538,8 +1542,10 @@ Cache<TagStore>::handleFill(PacketPtr pkt, BlkType *blk,
     DPRINTF(Cache, "Block addr %x (%s) moving from state %x to %s\n",
             addr, is_secure ? "s" : "ns", old_state, blk->print());
 
-    // if we got new data, copy it in
+    // if we got new data, copy it in (checking for a read response
+    // and a response that has data is the same in the end)
     if (pkt->isRead()) {
+        assert(pkt->hasData());
         std::memcpy(blk->data, pkt->getConstPtr<uint8_t>(), blkSize);
     }
 
diff --git a/src/mem/packet.cc b/src/mem/packet.cc
index 758770824..09b612ad0 100644
--- a/src/mem/packet.cc
+++ b/src/mem/packet.cc
@@ -174,7 +174,7 @@ MemCmd::commandInfo[] =
 
 bool
 Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size,
-                        uint8_t *data)
+                        uint8_t *_data)
 {
     Addr func_start = getAddr();
     Addr func_end   = getAddr() + getSize() - 1;
@@ -189,12 +189,14 @@ Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size,
 
     // check print first since it doesn't require data
     if (isPrint()) {
+        assert(!_data);
         safe_cast<PrintReqState*>(senderState)->printObj(obj);
         return false;
     }
 
-    // if there's no data, there's no need to look further
-    if (!data) {
+    // we allow the caller to pass NULL to signify the other packet
+    // has no data
+    if (!_data) {
         return false;
     }
 
@@ -204,7 +206,9 @@ Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size,
 
     if (isRead()) {
         if (func_start >= val_start && func_end <= val_end) {
-            memcpy(getPtr<uint8_t>(), data + offset, getSize());
+            memcpy(getPtr<uint8_t>(), _data + offset, getSize());
+            // complete overlap, and as the current packet is a read
+            // we are done
             return true;
         } else {
             // Offsets and sizes to copy in case of partial overlap
@@ -271,7 +275,7 @@ Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size,
 
                     // copy the first half
                     uint8_t *dest = getPtr<uint8_t>() + func_offset;
-                    uint8_t *src = data + val_offset;
+                    uint8_t *src = _data + val_offset;
                     memcpy(dest, src, (bytesValidStart - func_offset));
 
                     // re-calc the offsets and indices to do the copy
@@ -293,7 +297,7 @@ Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size,
 
             // copy partial data into the packet's data array
             uint8_t *dest = getPtr<uint8_t>() + func_offset;
-            uint8_t *src = data + val_offset;
+            uint8_t *src = _data + val_offset;
             memcpy(dest, src, overlap_size);
 
             // check if we're done filling the functional access
@@ -302,11 +306,11 @@ Packet::checkFunctional(Printable *obj, Addr addr, bool is_secure, int size,
         }
     } else if (isWrite()) {
         if (offset >= 0) {
-            memcpy(data + offset, getConstPtr<uint8_t>(),
+            memcpy(_data + offset, getConstPtr<uint8_t>(),
                    (min(func_end, val_end) - func_start) + 1);
         } else {
             // val_start > func_start
-            memcpy(data, getConstPtr<uint8_t>() - offset,
+            memcpy(_data, getConstPtr<uint8_t>() - offset,
                    (min(func_end, val_end) - val_start) + 1);
         }
     } else {
diff --git a/src/mem/packet.hh b/src/mem/packet.hh
index 357134c75..d0b210975 100644
--- a/src/mem/packet.hh
+++ b/src/mem/packet.hh
@@ -185,6 +185,12 @@ class MemCmd
     bool needsExclusive() const { return testCmdAttrib(NeedsExclusive); }
     bool needsResponse() const  { return testCmdAttrib(NeedsResponse); }
     bool isInvalidate() const   { return testCmdAttrib(IsInvalidate); }
+
+    /**
+     * Check if this particular packet type carries payload data. Note
+     * that this does not reflect if the data pointer of the packet is
+     * valid or not.
+     */
     bool hasData() const        { return testCmdAttrib(HasData); }
     bool isLLSC() const         { return testCmdAttrib(IsLlsc); }
     bool isSWPrefetch() const   { return testCmdAttrib(IsSWPrefetch); }
@@ -918,28 +924,37 @@ class Packet : public Printable
     }
 
     /**
-     * Check a functional request against a memory value represented
-     * by a base/size pair and an associated data array.  If the
-     * functional request is a read, it may be satisfied by the memory
-     * value.  If the functional request is a write, it may update the
-     * memory value.
-     */
-    bool checkFunctional(Printable *obj, Addr base, bool is_secure, int size,
-                         uint8_t *data);
-
-    /**
      * Check a functional request against a memory value stored in
-     * another packet (i.e. an in-transit request or response).
+     * another packet (i.e. an in-transit request or
+     * response). Returns true if the current packet is a read, and
+     * the other packet provides the data, which is then copied to the
+     * current packet. If the current packet is a write, and the other
+     * packet intersects this one, then we update the data
+     * accordingly.
      */
     bool
-    checkFunctional(PacketPtr other) 
+    checkFunctional(PacketPtr other)
     {
-        uint8_t *data = other->hasData() ? other->getPtr<uint8_t>() : NULL;
+        // all packets that are carrying a payload should have a valid
+        // data pointer
         return checkFunctional(other, other->getAddr(), other->isSecure(),
-                               other->getSize(), data);
+                               other->getSize(),
+                               other->hasData() ?
+                               other->getPtr<uint8_t>() : NULL);
     }
 
     /**
+     * Check a functional request against a memory value represented
+     * by a base/size pair and an associated data array. If the
+     * current packet is a read, it may be satisfied by the memory
+     * value. If the current packet is a write, it may update the
+     * memory value.
+     */
+    bool
+    checkFunctional(Printable *obj, Addr base, bool is_secure, int size,
+                    uint8_t *_data);
+
+    /**
      * Push label for PrintReq (safe to call unconditionally).
      */
     void