Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/6637 )
Change subject: Add unsigned-integer overflow checking to UBSAN ...................................................................... Patch Set 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/6637/1/CMakeLists.txt File CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/6637/1/CMakeLists.txt@343 PS1, Line 343: set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11") > Nit: split across separate lines, and maybe add comments for the non-obviou Done http://gerrit.cloudera.org:8080/#/c/6637/1/build-support/ubsan-blacklist.txt File build-support/ubsan-blacklist.txt: PS1: > Copyright? Check the other suppression files. Done http://gerrit.cloudera.org:8080/#/c/6637/1/build-support/ubsan-blacklist.txt@9 PS1, Line 9: # http://www.apache.org/licenses/LICENSE-2.0 > Could you add a comment explaining the syntax of the file? I know I could p I added a link to the LLVM docs for the syntax. 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 Done 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: UInt8DataGenerator() {} > is this OK on the older compilers we support like gcc 4.8? If not maybe we Done 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: } : > I'm skeptical of this fix -- doesn't this change it from going in reverse o Done 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: MonoDelta HybridClock::GetPhysicalComponentDifference(Timestamp lhs, Timestamp rhs) const { > hrm, I think this should be fixed by casting to int64 before subtraction, n Done http://gerrit.cloudera.org:8080/#/c/6637/2/src/kudu/clock/hybrid_clock.cc@466 PS2, Line 466: return TimestampFromMicrosecondsAndLogicalValue(new_physical, old_logical); > same, these should probably be int64s Done 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: } > update above comment if you want to make this change Done http://gerrit.cloudera.org:8080/#/c/6637/1/src/kudu/util/alignment.h File src/kudu/util/alignment.h: http://gerrit.cloudera.org:8080/#/c/6637/1/src/kudu/util/alignment.h@26 PS1, Line 26: #define KUDU_ALIGN_UP(x, align) (((x) + ((align) - 1)) & (~(align) + 1)) > Why this change? And why not an equivalent change to KUDU_ALIGN_DOWN? Because `-(align)` is really `0 - align`, which is overflow if align is positive or non-zero (align is typically a unsigned integer of type size_t). I've updated both. Note that ones-complement (~ operator) is not considered overflow, and (~x + 1) == -x when overflow happens as you would expect. http://gerrit.cloudera.org:8080/#/c/6637/1/src/kudu/util/bitmap-test.cc File src/kudu/util/bitmap-test.cc: http://gerrit.cloudera.org:8080/#/c/6637/1/src/kudu/util/bitmap-test.cc@175 PS1, Line 175: BitmapChange(bm, i, i & 3); > More idiomatic as a for loop? Done 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.user -= times_.user; > maybe better to just change the type to signed integer here. That way if th Done -- 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: 3 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 18 Jan 2018 04:05:54 +0000 Gerrit-HasComments: Yes
