Fix overflow in sparkbar function #48121
Conversation
|
My memory tells it is like 3rd time we try to fix this function... |
| @@ -43,7 +43,16 @@ struct AggregateFunctionSparkbarData | |||
|
|
|||
| auto [it, inserted] = points.insert({x, y}); | |||
| if (!inserted) | |||
| it->getMapped() += y; | |||
| { | |||
| if (std::numeric_limits<Y>::max() - it->getMapped() > y) | |||
There was a problem hiding this comment.
Use arithmeticOverflow.h, it should be more efficient.
There was a problem hiding this comment.
In fact, it is ok to annotate it with NO_SANITIZE_UNDEFINED, because the signed integer overflow is UB from the C++ compiler's standpoint but not from the CPU standpoint. We use it in other arithmetic operations, such as plus
There was a problem hiding this comment.
@alexey-milovidov Unsigned overflow is also possible. In such case, saturating is a more expected result than losing values with passing through zero, imho. The arithmeticOverflow.h allows only overflow. Should we introduce functions for saturating arithmetic?
Another approach is to use UInt64 as a counter type for all smaller types (and handle only UInt64 overflow).
There was a problem hiding this comment.
UPD: switched implementation to common:addOverflow and common::mulOverflow with checking the result and saturating
| @@ -161,7 +171,7 @@ class AggregateFunctionSparkbar final | |||
| } | |||
|
|
|||
| PaddedPODArray<Y> histogram(width, 0); | |||
| PaddedPODArray<UInt64> fhistogram(width, 0); | |||
vdimir
force-pushed
the
vdimir/sparkbar-overflow-fix
branch
from
c2a7735
to
ee3a840
Compare
vdimir
force-pushed
the
vdimir/sparkbar-overflow-fix
branch
from
ee3a840
to
2d18689
Compare
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Documentation entry for user-facing changes