folly: ubsan: disable integer overflow tests in Histogram
authorLucian Grijincu <lucian@fb.com>
Tue, 10 May 2016 06:00:19 +0000 (23:00 -0700)
committerFacebook Github Bot 5 <facebook-github-bot-5-bot@fb.com>
Tue, 10 May 2016 06:05:19 +0000 (23:05 -0700)
Summary:
It would be nice to fix, but it's going to be easier to do when folly
support is for GCC>=5 which adds `__builtin_add_overflow`.

For now disable the check as the Histgram code handles overflow
correctly and has tests for these cases.

Fixes need to be done for both float and integer types as Histogram is used with both.

Reviewed By: meyering

Differential Revision: D3280260

fbshipit-source-id: 56e524c517566a4547346859be7770eda2acee96

folly/stats/Histogram.h

index 0b9baba1246f492c0aa281b578a6df8a73a1c433..f6fbea717c0b5035d0bfa26f2e970a589eeb9caf 100644 (file)
@@ -23,6 +23,7 @@
 #include <vector>
 #include <stdexcept>
 
+#include <folly/CPortability.h>
 #include <folly/detail/Stats.h>
 
 namespace folly {
@@ -242,17 +243,24 @@ class Histogram {
     : buckets_(bucketSize, min, max, Bucket()) {}
 
   /* Add a data point to the histogram */
-  void addValue(ValueType value) {
+  void addValue(ValueType value) UBSAN_DISABLE("signed-integer-overflow")
+      UBSAN_DISABLE("unsigned-integer-overflow") {
     Bucket& bucket = buckets_.getByValue(value);
-    // TODO: It would be nice to handle overflow here.
+    // NOTE: Overflow is handled elsewhere and tests check this
+    // behavior (see HistogramTest.cpp TestOverflow* tests).
+    // TODO: It would be nice to handle overflow here and redesign this class.
     bucket.sum += value;
     bucket.count += 1;
   }
 
   /* Add multiple same data points to the histogram */
-  void addRepeatedValue(ValueType value, uint64_t nSamples) {
+  void addRepeatedValue(ValueType value, uint64_t nSamples)
+      UBSAN_DISABLE("signed-integer-overflow")
+          UBSAN_DISABLE("unsigned-integer-overflow") {
     Bucket& bucket = buckets_.getByValue(value);
-    // TODO: It would be nice to handle overflow here.
+    // NOTE: Overflow is handled elsewhere and tests check this
+    // behavior (see HistogramTest.cpp TestOverflow* tests).
+    // TODO: It would be nice to handle overflow here and redesign this class.
     bucket.sum += value * nSamples;
     bucket.count += nSamples;
   }
@@ -264,9 +272,12 @@ class Histogram {
    * had previously been added to the histogram; it merely subtracts the
    * requested value from the appropriate bucket's sum.
    */
-  void removeValue(ValueType value) {
+  void removeValue(ValueType value) UBSAN_DISABLE("signed-integer-overflow")
+      UBSAN_DISABLE("unsigned-integer-overflow") {
     Bucket& bucket = buckets_.getByValue(value);
-    // TODO: It would be nice to handle overflow here.
+    // NOTE: Overflow is handled elsewhere and tests check this
+    // behavior (see HistogramTest.cpp TestOverflow* tests).
+    // TODO: It would be nice to handle overflow here and redesign this class.
     if (bucket.count > 0) {
       bucket.sum -= value;
       bucket.count -= 1;