base: Initialize storage params on constructor

Force error checking on storage params by imposing it on construction.

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

diff --git a/src/base/statistics.hh b/src/base/statistics.hh
index b1f27c72617dd16d8b620178f7af894cec7e0332..a1994c8431f8fd67ae624d182cf26d08fa19e20c 100644 (file)
--- a/src/base/statistics.hh
+++ b/src/base/statistics.hh
@@ -2005,14 +2005,7 @@ class Distribution : public DistBase<Distribution, DistStor>
     Distribution &
     init(Counter min, Counter max, Counter bkt)
     {
-        DistStor::Params *params = new DistStor::Params;
-        params->min = min;
-        params->max = max;
-        params->bucket_size = bkt;
-        // Division by zero is especially serious in an Aarch64 host,
-        // where it gets rounded to allocate 32GiB RAM.
-        assert(bkt > 0);
-        params->buckets = (size_type)ceil((max - min + 1.0) / bkt);
+        DistStor::Params *params = new DistStor::Params(min, max, bkt);
         this->setParams(params);
         this->doInit();
         return this->self();
@@ -2040,8 +2033,7 @@ class Histogram : public DistBase<Histogram, HistStor>
     Histogram &
     init(size_type size)
     {
-        HistStor::Params *params = new HistStor::Params;
-        params->buckets = size;
+        HistStor::Params *params = new HistStor::Params(size);
         this->setParams(params);
         this->doInit();
         return this->self();
@@ -2112,11 +2104,7 @@ class VectorDistribution : public VectorDistBase<VectorDistribution, DistStor>
     VectorDistribution &
     init(size_type size, Counter min, Counter max, Counter bkt)
     {
-        DistStor::Params *params = new DistStor::Params;
-        params->min = min;
-        params->max = max;
-        params->bucket_size = bkt;
-        params->buckets = (size_type)ceil((max - min + 1.0) / bkt);
+        DistStor::Params *params = new DistStor::Params(min, max, bkt);
         this->setParams(params);
         this->doInit(size);
         return this->self();

diff --git a/src/base/stats/storage.cc b/src/base/stats/storage.cc
index 02e8716498ab3e8025bab66125cbd7f6586d4aba..6e2a7f2f96d3559878bb6d35a11bd85994cb3962 100644 (file)
--- a/src/base/stats/storage.cc
+++ b/src/base/stats/storage.cc
@@ -54,10 +54,7 @@ DistStor::sample(Counter val, int number)
     else if (val > max_track)
         overflow += number;
     else {
-        size_type index =
-            (size_type)std::floor((val - min_track) / bucket_size);
-        assert(index < size());
-        cvec[index] += number;
+        cvec[std::floor((val - min_track) / bucket_size)] += number;
     }
 
     if (val < min_val)

diff --git a/src/base/stats/storage.hh b/src/base/stats/storage.hh
index 66565bbd9609aba7ea98dc61931d365917c5f005..2ee1eb2f1e86c85d64b4b72b225c26264b4d27f4 100644 (file)
--- a/src/base/stats/storage.hh
+++ b/src/base/stats/storage.hh
@@ -31,6 +31,7 @@
 #define __BASE_STATS_STORAGE_HH__
 
 #include <cassert>
+#include <cmath>
 
 #include "base/cast.hh"
 #include "base/logging.hh"
@@ -265,8 +266,19 @@ class DistStor
         /** The number of buckets. Equal to (max-min)/bucket_size. */
         size_type buckets;
 
-        Params() : DistParams(Dist), min(0), max(0), bucket_size(0),
-                   buckets(0) {}
+        Params(Counter _min, Counter _max, Counter _bucket_size)
+          : DistParams(Dist), min(_min), max(_max), bucket_size(_bucket_size),
+            buckets(0)
+        {
+            fatal_if(bucket_size <= 0,
+                "Bucket size (%f) must be greater than zero", bucket_size);
+            warn_if(std::floor((max - min + 1.0) / bucket_size) !=
+                std::ceil((max - min + 1.0) / bucket_size),
+                "Bucket size (%f) does not divide range [%f:%f] into equal-" \
+                "sized buckets. Rounding up.", bucket_size, min + 1.0, max);
+
+            buckets = std::ceil((max - min + 1.0) / bucket_size);
+        }
     };
 
     DistStor(Info *info)
@@ -446,14 +458,18 @@ class HistStor
         /** The number of buckets. */
         size_type buckets;
 
-        Params() : DistParams(Hist), buckets(0) {}
+        Params(size_type _buckets)
+          : DistParams(Hist)
+        {
+            fatal_if(_buckets < 2,
+                "There must be at least two buckets in a histogram");
+            buckets = _buckets;
+        }
     };
 
     HistStor(Info *info)
         : cvec(safe_cast<const Params *>(info->storageParams)->buckets)
     {
-        fatal_if(cvec.size() == 1,
-            "There must be at least two buckets in a histogram");
         reset(info);
     }
 

diff --git a/src/base/stats/storage.test.cc b/src/base/stats/storage.test.cc
index 8a4f6ed5e05091a5b9e058cceab00ec160d88368..78df23d0689647b611212ed3e8a13dd2b2b92df9 100644 (file)
--- a/src/base/stats/storage.test.cc
+++ b/src/base/stats/storage.test.cc
@@ -266,18 +266,12 @@ TEST(StatsAvgStorTest, ZeroReset)
     ASSERT_FALSE(stor.zero());
 }
 
-/**
- * Test that an assertion is thrown when no bucket size is provided before
- * sampling.
- */
-TEST(StatsDistStorDeathTest, NoBucketSize)
+/** Test that an assertion is thrown when bucket size is 0. */
+TEST(StatsDistStorDeathTest, BucketSize0)
 {
-    Stats::Counter val = 10;
-    Stats::Counter num_samples = 5;
-    Stats::DistStor::Params params;
-    MockInfo info(&params);
-    Stats::DistStor stor(&info);
-    ASSERT_DEATH(stor.sample(val, num_samples), ".+");
+    testing::internal::CaptureStderr();
+    EXPECT_ANY_THROW(Stats::DistStor::Params params(0, 5, 0));
+    testing::internal::GetCapturedStderr();
 }
 
 /**
@@ -287,8 +281,7 @@ TEST(StatsDistStorDeathTest, NoBucketSize)
  */
 TEST(StatsDistStorTest, ZeroReset)
 {
-    Stats::DistStor::Params params;
-    params.bucket_size = 10;
+    Stats::DistStor::Params params(0, 99, 10);
     MockInfo info(&params);
     Stats::DistStor stor(&info);
     Stats::Counter val = 10;
@@ -315,9 +308,7 @@ TEST(StatsDistStorTest, Size)
     Stats::Counter size = 20;
     Stats::DistData data;
 
-    Stats::DistStor::Params params;
-    params.bucket_size = 1;
-    params.buckets = size;
+    Stats::DistStor::Params params(0, 19, 1);
     MockInfo info(&params);
     Stats::DistStor stor(&info);
 
@@ -406,11 +397,7 @@ prepareCheckDistStor(Stats::DistStor::Params& params, ValueSamples* values,
 /** Test setting and getting value from storage. */
 TEST(StatsDistStorTest, SamplePrepareSingle)
 {
-    Stats::DistStor::Params params;
-    params.min = 0;
-    params.max = 99;
-    params.bucket_size = 5;
-    params.buckets = 20;
+    Stats::DistStor::Params params(0, 99, 5);
 
     ValueSamples values[] = {{10, 5}};
     int num_values = sizeof(values) / sizeof(ValueSamples);
@@ -433,11 +420,7 @@ TEST(StatsDistStorTest, SamplePrepareSingle)
 /** Test setting and getting value from storage with multiple values. */
 TEST(StatsDistStorTest, SamplePrepareMultiple)
 {
-    Stats::DistStor::Params params;
-    params.min = 0;
-    params.max = 99;
-    params.bucket_size = 5;
-    params.buckets = 20;
+    Stats::DistStor::Params params(0, 99, 5);
 
     // There are 20 buckets: [0,5[, [5,10[, [10,15[, ..., [95,100[.
     // We test that values that pass the maximum bucket value (1234, 12345678,
@@ -474,11 +457,7 @@ TEST(StatsDistStorTest, SamplePrepareMultiple)
 /** Test resetting storage. */
 TEST(StatsDistStorTest, Reset)
 {
-    Stats::DistStor::Params params;
-    params.min = 0;
-    params.max = 99;
-    params.bucket_size = 5;
-    params.buckets = 20;
+    Stats::DistStor::Params params(0, 99, 5);
     MockInfo info(&params);
     Stats::DistStor stor(&info);
 
@@ -513,20 +492,20 @@ TEST(StatsDistStorTest, Reset)
     checkExpectedDistData(data, expected_data, true);
 }
 
-/**
- * Test that an assertion is thrown when no bucket size is provided before
- * sampling.
- */
-TEST(StatsHistStorDeathTest, NoBucketSize)
+/** Test that an assertion is thrown when not enough buckets are provided. */
+TEST(StatsHistStorDeathTest, NotEnoughBuckets0)
 {
-    Stats::Counter val = 10;
-    Stats::Counter num_samples = 5;
+    testing::internal::CaptureStderr();
+    EXPECT_ANY_THROW(Stats::HistStor::Params params(0));
+    testing::internal::GetCapturedStderr();
+}
 
-    // If no bucket size is specified, it is 0 by default
-    Stats::HistStor::Params params;
-    MockInfo info(&params);
-    Stats::HistStor stor(&info);
-    ASSERT_DEATH(stor.sample(val, num_samples), ".+");
+/** Test that an assertion is thrown when not enough buckets are provided. */
+TEST(StatsHistStorDeathTest, NotEnoughBuckets1)
+{
+    testing::internal::CaptureStderr();
+    EXPECT_ANY_THROW(Stats::HistStor::Params params(1));
+    testing::internal::GetCapturedStderr();
 }
 
 /**
@@ -536,8 +515,7 @@ TEST(StatsHistStorDeathTest, NoBucketSize)
  */
 TEST(StatsHistStorTest, ZeroReset)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 10;
+    Stats::HistStor::Params params(10);
     MockInfo info(&params);
     Stats::HistStor stor(&info);
     Stats::Counter val = 10;
@@ -564,24 +542,8 @@ TEST(StatsHistStorTest, Size)
     Stats::DistData data;
     Stats::size_type sizes[] = {2, 10, 1234};
 
-    // If no bucket size is specified, it is 0 by default
-    {
-        Stats::HistStor::Params params;
-        MockInfo info(&params);
-        Stats::HistStor stor(&info);
-
-        ASSERT_EQ(stor.size(), 0);
-        stor.prepare(&info, data);
-        ASSERT_EQ(stor.size(), 0);
-        stor.reset(&info);
-        ASSERT_EQ(stor.size(), 0);
-        stor.zero();
-        ASSERT_EQ(stor.size(), 0);
-    }
-
     for (int i = 0; i < (sizeof(sizes) / sizeof(Stats::size_type)); i++) {
-        Stats::HistStor::Params params;
-        params.buckets = sizes[i];
+        Stats::HistStor::Params params(sizes[i]);
         MockInfo info(&params);
         Stats::HistStor stor(&info);
 
@@ -651,8 +613,7 @@ prepareCheckHistStor(Stats::HistStor::Params& params, ValueSamples* values,
  */
 TEST(StatsHistStorTest, SamplePrepareFit)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 4;
+    Stats::HistStor::Params params(4);
 
     // Setup expected data for the hand-carved values given. The final buckets
     // will be divided at:
@@ -680,8 +641,7 @@ TEST(StatsHistStorTest, SamplePrepareFit)
  */
 TEST(StatsHistStorTest, SamplePrepareSingleGrowUp)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 4;
+    Stats::HistStor::Params params(4);
 
     // Setup expected data for the hand-carved values given. Since there
     // are four buckets, and the highest value is 4, the bucket size will
@@ -710,8 +670,7 @@ TEST(StatsHistStorTest, SamplePrepareSingleGrowUp)
  */
 TEST(StatsHistStorTest, SamplePrepareMultipleGrowUp)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 4;
+    Stats::HistStor::Params params(4);
 
     // Setup expected data for the hand-carved values given. Since there
     // are four buckets, and the highest value is 4, the bucket size will
@@ -741,8 +700,7 @@ TEST(StatsHistStorTest, SamplePrepareMultipleGrowUp)
  */
 TEST(StatsHistStorTest, SamplePrepareGrowDownOddBuckets)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 5;
+    Stats::HistStor::Params params(5);
 
     // Setup expected data for the hand-carved values given. Since there
     // is a negative value, the min bucket will change, and the bucket size
@@ -774,8 +732,7 @@ TEST(StatsHistStorTest, SamplePrepareGrowDownOddBuckets)
  */
 TEST(StatsHistStorTest, SamplePrepareGrowDownEvenBuckets)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 4;
+    Stats::HistStor::Params params(4);
 
     // Setup expected data for the hand-carved values given. Since there
     // is a negative value, the min bucket will change, and the bucket size
@@ -805,8 +762,7 @@ TEST(StatsHistStorTest, SamplePrepareGrowDownEvenBuckets)
  */
 TEST(StatsHistStorTest, SamplePrepareGrowDownGrowOutOddBuckets)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 5;
+    Stats::HistStor::Params params(5);
 
     // Setup expected data for the hand-carved values given. Since there
     // is a negative value, the min bucket will change, and the bucket size
@@ -838,8 +794,7 @@ TEST(StatsHistStorTest, SamplePrepareGrowDownGrowOutOddBuckets)
  */
 TEST(StatsHistStorTest, SamplePrepareGrowDownGrowOutEvenBuckets)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 4;
+    Stats::HistStor::Params params(4);
 
     // Setup expected data for the hand-carved values given. Since there
     // is a negative value, the min bucket will change, and the bucket size
@@ -870,8 +825,7 @@ TEST(StatsHistStorTest, SamplePrepareGrowDownGrowOutEvenBuckets)
  */
 TEST(StatsHistStorTest, SamplePrepareMultipleGrowOddBuckets)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 5;
+    Stats::HistStor::Params params(5);
 
     // Setup expected data for the hand-carved values given. This adds quite
     // a few positive and negative samples, and the bucket size will grow to
@@ -904,8 +858,7 @@ TEST(StatsHistStorTest, SamplePrepareMultipleGrowOddBuckets)
  */
 TEST(StatsHistStorTest, SamplePrepareMultipleGrowEvenBuckets)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 4;
+    Stats::HistStor::Params params(4);
 
     // Setup expected data for the hand-carved values given. This adds quite
     // a few positive and negative samples, and the bucket size will grow to
@@ -932,8 +885,7 @@ TEST(StatsHistStorTest, SamplePrepareMultipleGrowEvenBuckets)
 /** Test resetting storage. */
 TEST(StatsHistStorTest, Reset)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 4;
+    Stats::HistStor::Params params(4);
     MockInfo info(&params);
     Stats::HistStor stor(&info);
 
@@ -964,13 +916,11 @@ TEST(StatsHistStorTest, Reset)
 /** Test whether adding storages with different sizes triggers an assertion. */
 TEST(StatsHistStorDeathTest, AddDifferentSize)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 4;
+    Stats::HistStor::Params params(4);
     MockInfo info(&params);
     Stats::HistStor stor(&info);
 
-    Stats::HistStor::Params params2;
-    params2.buckets = 5;
+    Stats::HistStor::Params params2(5);
     MockInfo info2(&params2);
     Stats::HistStor stor2(&info2);
 
@@ -980,15 +930,13 @@ TEST(StatsHistStorDeathTest, AddDifferentSize)
 /** Test whether adding storages with different min triggers an assertion. */
 TEST(StatsHistStorDeathTest, AddDifferentMin)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 4;
+    Stats::HistStor::Params params(4);
     MockInfo info(&params);
     Stats::HistStor stor(&info);
     stor.sample(-1, 3);
 
     // On creation, the storage's min is zero
-    Stats::HistStor::Params params2;
-    params2.buckets = 4;
+    Stats::HistStor::Params params2(4);
     MockInfo info2(&params2);
     Stats::HistStor stor2(&info2);
 
@@ -998,8 +946,7 @@ TEST(StatsHistStorDeathTest, AddDifferentMin)
 /** Test merging two histograms. */
 TEST(StatsHistStorTest, Add)
 {
-    Stats::HistStor::Params params;
-    params.buckets = 4;
+    Stats::HistStor::Params params(4);
     MockInfo info(&params);
 
     // Setup first storage. Buckets are: