Adar Dembo has posted comments on this change. Change subject: WIP: Add unsigned-integer overflow checking to UBSAN ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/6637/1//COMMIT_MSG Commit Message: PS1, Line 9: WIP because I haven't fixed any of these issues, only added : ignore annotations (a handful of them had to be fixed because they were : inside macro calls which can't be annotated). +1, so maybe you can go through and, for each annotation, either add a comment explaining why it's needed, or remove it and fix the underlying issue? http://gerrit.cloudera.org:8080/#/c/6637/1/CMakeLists.txt File CMakeLists.txt: Line 343: set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=undefined,integer -fsanitize-blacklist=${CMAKE_CURRENT_SOURCE_DIR}/build-support/ubsan-blacklist.txt -fno-sanitize=alignment,vptr -fno-sanitize-recover") Nit: split across separate lines, and maybe add comments for the non-obvious additions? http://gerrit.cloudera.org:8080/#/c/6637/1/build-support/ubsan-blacklist.txt File build-support/ubsan-blacklist.txt: Copyright? Check the other suppression files. Line 9: Could you add a comment explaining the syntax of the file? I know I could probably figure it out by reading the LLVM docs, but since you've gone through the trouble, a couple lines here would be helpful. Basically, just explain that src/fun are the only two options, and that fun must be mangled. http://gerrit.cloudera.org:8080/#/c/6637/1/src/kudu/util/alignment.h File src/kudu/util/alignment.h: 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? http://gerrit.cloudera.org:8080/#/c/6637/1/src/kudu/util/bitmap-test.cc File src/kudu/util/bitmap-test.cc: Line 175: while (num_bits > 0) { More idiomatic as a for loop? -- To view, visit http://gerrit.cloudera.org:8080/6637 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib0712f5307dcbab7fdffbe2364e1fdb691d3aaab Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
