Histogram APIs and truncation |
|
Issue descriptionTo better identify coding errors I'm iterating on a patch where "-Wconversion" compiler flag is applied in blink. This identifies narrowing error from 64 bit values to 32 bit values and it has identified a few histograms which have issues: Specifically consider this example: size_t a = 0xbaadbeef00000010LL; histogram->AddCount(20, a); You might expect that this adds a count of the max value a histogram can support (which is INT_MAX - 1) but it will actually add a count of 0x10 because of the implicit narrowing. The correct approach here would be to: size_t a = 0xbaadbeef00000010LL; histogram->AddCount(20, saturated_cast<int>(a)); But this is problematic to actually get correct everywhere as some people might just do a static_cast<int> which would be incorrect. More problematic usages of the API are also seen with the ScaledLinearHistogram which lead the user to think that the domain of the value is scaled and then logged. But the value is first truncated and then scaled. Problematic usages are seen in various spots: https://chromium.googlesource.com/chromium/src/+/bf0112123d055721209dfe0c0f24b5774918ea9e/components/data_use_measurement/core/data_use_measurement.cc#438 This code uses a size_t member and scales it by 1024; which conceptually a consumer of this API would expect that values larger than 2gb could be counted but it can't because of the truncation. As well the truncating cast vs the saturated cast can cause incorrect counts.
,
Jul 6
I won't have an entire list until I fix all the errors in blink. There are many histograms probably >30 in blink alone. But there are thousands of narrowing errors in blink so I don't know how long it will take. So are you indicating that each callsite should be switched to use saturated_cast? |
|
►
Sign in to add a comment |
|
Comment 1 by rkaplow@chromium.org
, Jul 6