base: Fix negative op-assign of SatCounter

The value of the add and subtract assignment operations can be negative,
and this was not being handled properly previously. Regarding shift
assignment, the standard says it is undefined behaviour if a negative
number is given, so add assertions for these cases.

Change-Id: I2f1e4143c6385caa80fb25f84ca8edb0ca7e62b7
Signed-off-by: Daniel R. Carvalho <odanrc@yahoo.com.br>
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/23664
Reviewed-by: Bobby R. Bruce <bbruce@ucdavis.edu>
Reviewed-by: Jason Lowe-Power <jason@lowepower.com>
Maintainer: Jason Lowe-Power <jason@lowepower.com>
Tested-by: kokoro <noreply+kokoro@google.com>

diff --git a/src/base/sat_counter.hh b/src/base/sat_counter.hh
index 6aeca681d6da1de3cb75b349530c5aa0518837f2..946da4f26666009687298981c185323d79cfc4cf 100644 (file)
--- a/src/base/sat_counter.hh
+++ b/src/base/sat_counter.hh
@@ -44,6 +44,7 @@
 #ifndef __BASE_SAT_COUNTER_HH__
 #define __BASE_SAT_COUNTER_HH__
 
+#include <cassert>
 #include <cstdint>
 
 #include "base/logging.hh"
@@ -172,6 +173,7 @@ class SatCounter
     SatCounter&
     operator>>=(const int& shift)
     {
+        assert(shift >= 0);
         this->counter >>= shift;
         return *this;
     }
@@ -180,6 +182,7 @@ class SatCounter
     SatCounter&
     operator<<=(const int& shift)
     {
+        assert(shift >= 0);
         this->counter <<= shift;
         if (this->counter > maxVal) {
             this->counter = maxVal;
@@ -191,10 +194,14 @@ class SatCounter
     SatCounter&
     operator+=(const int& value)
     {
-        if (maxVal - this->counter >= value) {
-            this->counter += value;
+        if (value >= 0) {
+            if (maxVal - this->counter >= value) {
+                this->counter += value;
+            } else {
+                this->counter = maxVal;
+            }
         } else {
-            this->counter = maxVal;
+            *this -= -value;
         }
         return *this;
     }
@@ -203,10 +210,14 @@ class SatCounter
     SatCounter&
     operator-=(const int& value)
     {
-        if (this->counter > value) {
-            this->counter -= value;
+        if (value >= 0) {
+            if (this->counter > value) {
+                this->counter -= value;
+            } else {
+                this->counter = 0;
+            }
         } else {
-            this->counter = 0;
+            *this += -value;
         }
         return *this;
     }

diff --git a/src/base/sat_counter.test.cc b/src/base/sat_counter.test.cc
index 817f2c7ac5887ddb8580f84cda2c5316c7aa68df..d9a377f9fdd7ed870f48999b8b5aa28e65815a1a 100644 (file)
--- a/src/base/sat_counter.test.cc
+++ b/src/base/sat_counter.test.cc
@@ -28,6 +28,7 @@
  * Authors: Daniel Carvalho
  */
 
+#include <gtest/gtest-spi.h>
 #include <gtest/gtest.h>
 
 #include <utility>
@@ -184,6 +185,11 @@ TEST(SatCounterTest, Shift)
     ASSERT_EQ(counter, value);
     counter >>= saturated_counter;
     ASSERT_EQ(counter, 0);
+
+    // Make sure the counters cannot be shifted by negative numbers, since
+    // that is undefined behaviour
+    ASSERT_DEATH(counter >>= -1, "");
+    ASSERT_DEATH(counter <<= -1, "");
 }
 
 /**
@@ -319,3 +325,36 @@ TEST(SatCounterTest, AddSubAssignment)
     ASSERT_EQ(counter, 0);
 }
 
+/**
+ * Test add-assignment and subtract assignment using negative numbers.
+ */
+TEST(SatCounterTest, NegativeAddSubAssignment)
+{
+    const unsigned bits = 3;
+    const unsigned max_value = (1 << bits) - 1;
+    SatCounter counter(bits, max_value);
+    int value = max_value;
+
+    // Test add-assignment for a few negative values until zero is reached
+    counter += -2;
+    value += -2;
+    ASSERT_EQ(counter, value);
+    counter += -3;
+    value += -3;
+    ASSERT_EQ(counter, value);
+    counter += (int)-max_value;
+    value = 0;
+    ASSERT_EQ(counter, value);
+
+    // Test subtract-assignment for a few negative values until saturation
+    counter -= -2;
+    value -= -2;
+    ASSERT_EQ(counter, value);
+    counter -= -3;
+    value -= -3;
+    ASSERT_EQ(counter, value);
+    counter -= (int)-max_value;
+    value = max_value;
+    ASSERT_EQ(counter, value);
+}
+