impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] Add a build flag for the undefined behavior sanitizer, aka "ubsan".
Date Mon, 30 Jan 2017 18:34:48 GMT
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 <jbapple-impala@apache.org>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message