cpu: fix exec tracing memory corruption bug

Accessing traceData (to call setAddress() and/or setData())
after initiating a timing translation was causing crashes,
since a failed translation could delete the traceData
object before returning.

It turns out that there was never a need to access traceData
after initiating the translation, as the traced data was
always available earlier; this ordering was merely
historical.  Furthermore, traceData->setAddress() and
traceData->setData() were being called both from the CPU
model and the ISA definition, often redundantly.

This patch standardizes all setAddress and setData calls
for memory instructions to be in the CPU models and not
in the ISA definition.  It also moves those calls above
the translation calls to eliminate the crashes.

diff --git a/src/arch/alpha/isa/mem.isa b/src/arch/alpha/isa/mem.isa
index b1703221f561f0ce5f1dc6de9218311d1ae14a11..efff0eac7e2429880a23bbe7607d468564cb11a5 100644 (file)
--- a/src/arch/alpha/isa/mem.isa
+++ b/src/arch/alpha/isa/mem.isa
@@ -275,7 +275,6 @@ def template StoreExecute {{
         if (fault == NoFault) {
             fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
                               memAccessFlags, NULL);
-            if (traceData) { traceData->setData(Mem); }
         }
 
         if (fault == NoFault) {
@@ -310,7 +309,6 @@ def template StoreCondExecute {{
         if (fault == NoFault) {
             fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
                               memAccessFlags, &write_result);
-            if (traceData) { traceData->setData(Mem); }
         }
 
         if (fault == NoFault) {
@@ -344,7 +342,6 @@ def template StoreInitiateAcc {{
         if (fault == NoFault) {
             fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
                               memAccessFlags, NULL);
-            if (traceData) { traceData->setData(Mem); }
         }
 
         return fault;
@@ -478,9 +475,6 @@ def LoadStoreBase(name, Name, ea_code, memacc_code, mem_flags, inst_flags,
     mem_flags = makeList(mem_flags)
     inst_flags = makeList(inst_flags)
 
-    # add hook to get effective addresses into execution trace output.
-    ea_code += '\nif (traceData) { traceData->setAddr(EA); }\n'
-
     # Some CPU models execute the memory operation as an atomic unit,
     # while others want to separate them into an effective address
     # computation and a memory access operation.  As a result, we need

diff --git a/src/arch/arm/isa/formats/mem.isa b/src/arch/arm/isa/formats/mem.isa
index 0b0a4c9fa4b27c089e8469c8c83504c8636930e7..2f66ca54e0c4c0c5233bb841d555909b102f81fa 100644 (file)
--- a/src/arch/arm/isa/formats/mem.isa
+++ b/src/arch/arm/isa/formats/mem.isa
@@ -172,7 +172,6 @@ def template StoreExecute {{
             if (fault == NoFault) {
                 fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
                                   memAccessFlags, NULL);
-                if (traceData) { traceData->setData(Mem); }
             }
 
             if (fault == NoFault) {
@@ -204,7 +203,6 @@ def template StoreInitiateAcc {{
             if (fault == NoFault) {
                 fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
                                   memAccessFlags, NULL);
-                if (traceData) { traceData->setData(Mem); }
             }
 
             // Need to write back any potential address register update

diff --git a/src/arch/mips/isa/formats/mem.isa b/src/arch/mips/isa/formats/mem.isa
index 161a52b06910d8eba3804f1d1f5b2813d6ed6c04..411cc5fda5a5390f88f57291ce677d733841621c 100644 (file)
--- a/src/arch/mips/isa/formats/mem.isa
+++ b/src/arch/mips/isa/formats/mem.isa
@@ -305,7 +305,6 @@ def template StoreExecute {{
         if (fault == NoFault) {
             fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
                               memAccessFlags, NULL);
-            if (traceData) { traceData->setData(Mem); }
         }
 
         if (fault == NoFault) {
@@ -342,7 +341,6 @@ def template StoreFPExecute {{
         if (fault == NoFault) {
             fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
                               memAccessFlags, NULL);
-            if (traceData) { traceData->setData(Mem); }
         }
 
         if (fault == NoFault) {
@@ -377,7 +375,6 @@ def template StoreCondExecute {{
         if (fault == NoFault) {
             fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
                               memAccessFlags, &write_result);
-            if (traceData) { traceData->setData(Mem); }
         }
 
         if (fault == NoFault) {
@@ -411,7 +408,6 @@ def template StoreInitiateAcc {{
         if (fault == NoFault) {
             fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
                               memAccessFlags, NULL);
-            if (traceData) { traceData->setData(Mem); }
         }
 
         return fault;
@@ -435,8 +431,6 @@ def template StoreCompleteAcc {{
 
         if (fault == NoFault) {
             %(op_wb)s;
-
-            if (traceData) { traceData->setData(getMemData(xc, pkt)); }
         }
 
         return fault;
@@ -459,8 +453,6 @@ def template StoreCompleteAcc {{
 
         if (fault == NoFault) {
             %(op_wb)s;
-
-            if (traceData) { traceData->setData(getMemData(xc, pkt)); }
         }
 
         return fault;

diff --git a/src/arch/mips/isa/formats/util.isa b/src/arch/mips/isa/formats/util.isa
index a6edffedaa3f0b0867b2bbe4fbf4d1a376f52470..708338074b5504304bec029b988b7b4e11db867f 100644 (file)
--- a/src/arch/mips/isa/formats/util.isa
+++ b/src/arch/mips/isa/formats/util.isa
@@ -38,9 +38,6 @@ def LoadStoreBase(name, Name, ea_code, memacc_code, mem_flags, inst_flags,
     mem_flags = makeList(mem_flags)
     inst_flags = makeList(inst_flags)
 
-    # add hook to get effective addresses into execution trace output.
-    ea_code += '\nif (traceData) { traceData->setAddr(EA); }\n'
-
     # Some CPU models execute the memory operation as an atomic unit,
     # while others want to separate them into an effective address
     # computation and a memory access operation.  As a result, we need

diff --git a/src/arch/power/isa/formats/mem.isa b/src/arch/power/isa/formats/mem.isa
index 1be49c2f7f9414001963f348b5f1417d74929b7e..3bcf0633a8e642a66e142fcd83f93c5ce58204fe 100644 (file)
--- a/src/arch/power/isa/formats/mem.isa
+++ b/src/arch/power/isa/formats/mem.isa
@@ -166,7 +166,6 @@ def template StoreExecute {{
         if (fault == NoFault) {
             fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
                               memAccessFlags, NULL);
-            if (traceData) { traceData->setData(Mem); }
         }
 
         if (fault == NoFault) {
@@ -196,7 +195,6 @@ def template StoreInitiateAcc {{
         if (fault == NoFault) {
             fault = xc->write((uint%(mem_acc_size)d_t&)Mem, EA,
                               memAccessFlags, NULL);
-            if (traceData) { traceData->setData(Mem); }
         }
 
         // Need to write back any potential address register update

diff --git a/src/arch/power/isa/formats/util.isa b/src/arch/power/isa/formats/util.isa
index ab1e530b2bf867266f8285b76150eadbdd0f6353..8fd7f7daa2d91833b4dc15b653dc45e5ec2e63ff 100644 (file)
--- a/src/arch/power/isa/formats/util.isa
+++ b/src/arch/power/isa/formats/util.isa
@@ -97,9 +97,6 @@ def LoadStoreBase(name, Name, ea_code, memacc_code, mem_flags, inst_flags,
     mem_flags = makeList(mem_flags)
     inst_flags = makeList(inst_flags)
 
-    # add hook to get effective addresses into execution trace output.
-    ea_code += '\nif (traceData) { traceData->setAddr(EA); }\n'
-
     # Generate InstObjParams for the memory access.
     iop = InstObjParams(name, Name, base_class,
                         {'ea_code': ea_code,

diff --git a/src/cpu/inorder/resources/cache_unit.cc b/src/cpu/inorder/resources/cache_unit.cc
index 376ea8d26569b41d8261468332fd65dbe803cad4..d12f11a2cd548840a5b7fa8e49a10ea6183855b8 100644 (file)
--- a/src/cpu/inorder/resources/cache_unit.cc
+++ b/src/cpu/inorder/resources/cache_unit.cc
@@ -443,6 +443,10 @@ CacheUnit::read(DynInstPtr inst, Addr addr, T &data, unsigned flags)
     //The size of the data we're trying to read.
     int dataSize = sizeof(T);
 
+    if (inst->traceData) {
+        inst->traceData->setAddr(addr);
+    }
+
     if (inst->split2ndAccess) {     
         dataSize = inst->split2ndSize;
         cache_req->splitAccess = true;        
@@ -541,6 +545,11 @@ CacheUnit::write(DynInstPtr inst, T data, Addr addr, unsigned flags,
     //The size of the data we're trying to read.
     int dataSize = sizeof(T);
 
+    if (inst->traceData) {
+        inst->traceData->setAddr(addr);
+        inst->traceData->setData(data);
+    }
+
     if (inst->split2ndAccess) {     
         dataSize = inst->split2ndSize;
         cache_req->splitAccess = true;        

diff --git a/src/cpu/simple/atomic.cc b/src/cpu/simple/atomic.cc
index 05b4ca3e210bd3118364c498cef8add2ad08bc02..7740434d8b7bd5deec61e66e29076cb2f3844268 100644 (file)
--- a/src/cpu/simple/atomic.cc
+++ b/src/cpu/simple/atomic.cc
@@ -451,6 +451,7 @@ AtomicSimpleCPU::write(T data, Addr addr, unsigned flags, uint64_t *res)
 
     if (traceData) {
         traceData->setAddr(addr);
+        traceData->setData(data);
     }
 
     //The block size of our peer.
@@ -530,12 +531,6 @@ AtomicSimpleCPU::write(T data, Addr addr, unsigned flags, uint64_t *res)
         //stop now.
         if (fault != NoFault || secondAddr <= addr)
         {
-            // If the write needs to have a fault on the access, consider
-            // calling changeStatus() and changing it to "bad addr write"
-            // or something.
-            if (traceData) {
-                traceData->setData(gtoh(data));
-            }
             if (req->isLocked() && fault == NoFault) {
                 assert(locked);
                 locked = false;

diff --git a/src/cpu/simple/base.cc b/src/cpu/simple/base.cc
index 0104e1b1fee4d2dcea4f5f0a75969c8b2e20223e..17ba6a10b4484248d61a4e38d67c2eb26f3d7b81 100644 (file)
--- a/src/cpu/simple/base.cc
+++ b/src/cpu/simple/base.cc
@@ -205,6 +205,27 @@ change_thread_state(ThreadID tid, int activate, int priority)
 {
 }
 
+void
+BaseSimpleCPU::prefetch(Addr addr, unsigned flags)
+{
+    if (traceData) {
+        traceData->setAddr(addr);
+    }
+
+    // need to do this...
+}
+
+void
+BaseSimpleCPU::writeHint(Addr addr, int size, unsigned flags)
+{
+    if (traceData) {
+        traceData->setAddr(addr);
+    }
+
+    // need to do this...
+}
+
+
 Fault
 BaseSimpleCPU::copySrcTranslate(Addr src)
 {

diff --git a/src/cpu/simple/base.hh b/src/cpu/simple/base.hh
index 39961fb881b34823e45d6169751568c70cbb378d..87e21152128c0e5839e1446044d296790c66df2f 100644 (file)
--- a/src/cpu/simple/base.hh
+++ b/src/cpu/simple/base.hh
@@ -232,16 +232,8 @@ class BaseSimpleCPU : public BaseCPU
     Addr getEA()        { panic("BaseSimpleCPU::getEA() not implemented\n");
         M5_DUMMY_RETURN}
 
-    void prefetch(Addr addr, unsigned flags)
-    {
-        // need to do this...
-    }
-
-    void writeHint(Addr addr, int size, unsigned flags)
-    {
-        // need to do this...
-    }
-
+    void prefetch(Addr addr, unsigned flags);
+    void writeHint(Addr addr, int size, unsigned flags);
 
     Fault copySrcTranslate(Addr src);
 

diff --git a/src/cpu/simple/timing.cc b/src/cpu/simple/timing.cc
index 221cb0d0d9646dfbb18d3f5348adc7cf002ef72e..7583c09e6a07b9c3eec9e704120b2dd5f3d94831 100644 (file)
--- a/src/cpu/simple/timing.cc
+++ b/src/cpu/simple/timing.cc
@@ -426,6 +426,10 @@ TimingSimpleCPU::read(Addr addr, T &data, unsigned flags)
     int data_size = sizeof(T);
     BaseTLB::Mode mode = BaseTLB::Read;
 
+    if (traceData) {
+        traceData->setAddr(addr);
+    }
+
     RequestPtr req  = new Request(asid, addr, data_size,
                                   flags, pc, _cpuId, tid);
 
@@ -460,11 +464,6 @@ TimingSimpleCPU::read(Addr addr, T &data, unsigned flags)
         thread->dtb->translateTiming(req, tc, translation, mode);
     }
 
-    if (traceData) {
-        traceData->setData(data);
-        traceData->setAddr(addr);
-    }
-
     return NoFault;
 }
 
@@ -548,6 +547,11 @@ TimingSimpleCPU::write(T data, Addr addr, unsigned flags, uint64_t *res)
     int data_size = sizeof(T);
     BaseTLB::Mode mode = BaseTLB::Write;
 
+    if (traceData) {
+        traceData->setAddr(addr);
+        traceData->setData(data);
+    }
+
     RequestPtr req = new Request(asid, addr, data_size,
                                  flags, pc, _cpuId, tid);
 
@@ -584,13 +588,7 @@ TimingSimpleCPU::write(T data, Addr addr, unsigned flags, uint64_t *res)
         thread->dtb->translateTiming(req, tc, translation, mode);
     }
 
-    if (traceData) {
-        traceData->setAddr(req->getVaddr());
-        traceData->setData(data);
-    }
-
-    // If the write needs to have a fault on the access, consider calling
-    // changeStatus() and changing it to "bad addr write" or something.
+    // Translation faults will be returned via finishTranslation()
     return NoFault;
 }