Dan Hecht has posted comments on this change. Change subject: Add a build flag for the undefined behavior sanitizer, aka "ubsan". ......................................................................
Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/5082/4/be/src/exprs/operators-ir.cc File be/src/exprs/operators-ir.cc: Line 176: BINARY_OP_NUMERIC_TYPES(Subtract, -, Overflow::UnsignedDifference); for these, we eventually probably want to detect overflow and return NULL (when strict mode is disabled) or raise an error (when strict mode enabled). we'll need to be carful with performance, though. PS4, Line 177: Overflow::UnsignedProduct won't this give the wrong result for negative inputs? http://gerrit.cloudera.org:8080/#/c/5082/4/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: PS4, Line 83: int delta_scale = src_scale - dst_scale; not your change, but please fix this formatting. PS4, Line 95: Overflow::UnsignedProduct(result, mult); can't result be negative, in which case this isn't correct. also note that the callers already ignore result if *overflow is true. http://gerrit.cloudera.org:8080/#/c/5082/4/be/src/runtime/multi-precision.h File be/src/runtime/multi-precision.h: Line 111: scale = Overflow::UnsignedProduct(scale, base); given that callers won't actually use result if *overflow is true, do we need this? and even if we do, why not just make result and scale uint128_t so that the arithmetic is unsigned. http://gerrit.cloudera.org:8080/#/c/5082/4/be/src/util/overflow.h File be/src/util/overflow.h: Line 114: return AsUnsigned<std::multiplies<>>(x, y); this only gives the correct answer if x and y are non-negative. wouldn't it be better to fix the callsites to avoid overflow (I think most, if not all, already check overflow and ignore the result anyway). PS4, Line 119: must be share an unsigned type. garbled -- To view, visit http://gerrit.cloudera.org:8080/5082 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I88c7234bd7c5eb7404490a0913d90470c10835e7 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
