Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/6637 )
Change subject: WIP: Add unsigned-integer overflow checking to UBSAN ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/6637/1/build-support/ubsan-blacklist.txt File build-support/ubsan-blacklist.txt: http://gerrit.cloudera.org:8080/#/c/6637/1/build-support/ubsan-blacklist.txt@14 PS1, Line 14: # KIND, either express or implied. See the License for the typo http://gerrit.cloudera.org:8080/#/c/6637/2/src/kudu/cfile/cfile-test-base.h File src/kudu/cfile/cfile-test-base.h: http://gerrit.cloudera.org:8080/#/c/6637/2/src/kudu/cfile/cfile-test-base.h@163 PS2, Line 163: __attribute__((no_sanitize("integer"))) is this OK on the older compilers we support like gcc 4.8? If not maybe we should add ATTRIBUTE_NO_SANITIZE_INTEGER or something to port.h? http://gerrit.cloudera.org:8080/#/c/6637/1/src/kudu/cfile/index_btree.cc File src/kudu/cfile/index_btree.cc: http://gerrit.cloudera.org:8080/#/c/6637/1/src/kudu/cfile/index_btree.cc@178 PS1, Line 178: } hrm, surprised by this one. http://gerrit.cloudera.org:8080/#/c/6637/2/src/kudu/cfile/index_btree.cc File src/kudu/cfile/index_btree.cc: http://gerrit.cloudera.org:8080/#/c/6637/2/src/kudu/cfile/index_btree.cc@189 PS2, Line 189: for (const auto& seeked_index : seeked_indexes_) { : if (seeked_index->iter.HasNext()) I'm skeptical of this fix -- doesn't this change it from going in reverse order to going in forward order? I think given the context it might work either way, but would rather avoid a behavior change in this patch http://gerrit.cloudera.org:8080/#/c/6637/2/src/kudu/clock/hybrid_clock.cc File src/kudu/clock/hybrid_clock.cc: http://gerrit.cloudera.org:8080/#/c/6637/2/src/kudu/clock/hybrid_clock.cc@253 PS2, Line 253: // The result is expected to overflow if rhs > lhs. hrm, I think this should be fixed by casting to int64 before subtraction, no? http://gerrit.cloudera.org:8080/#/c/6637/2/src/kudu/clock/hybrid_clock.cc@466 PS2, Line 466: uint64_t new_physical = GetPhysicalValueMicros(original) + to_add.ToMicroseconds(); same, these should probably be int64s http://gerrit.cloudera.org:8080/#/c/6637/2/src/kudu/gutil/strings/numbers.cc File src/kudu/gutil/strings/numbers.cc: http://gerrit.cloudera.org:8080/#/c/6637/2/src/kudu/gutil/strings/numbers.cc@1020 PS2, Line 1020: u = ~u + 1; update above comment if you want to make this change http://gerrit.cloudera.org:8080/#/c/6637/2/src/kudu/util/stopwatch.h File src/kudu/util/stopwatch.h: http://gerrit.cloudera.org:8080/#/c/6637/2/src/kudu/util/stopwatch.h@223 PS2, Line 223: current.wall -= current.wall >= times_.wall ? times_.wall : current.wall; maybe better to just change the type to signed integer here. That way if the user's clock jumps backward we can actually see a negative value instead of clamping to 0. -- To view, visit http://gerrit.cloudera.org:8080/6637 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib0712f5307dcbab7fdffbe2364e1fdb691d3aaab Gerrit-Change-Number: 6637 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Thu, 18 Jan 2018 03:20:32 +0000 Gerrit-HasComments: Yes
