Small update: We've added clang tidy with mostly permissive rules. Feel free to have a look at the output of the build task to see if there's some code that could be cleaned up. Example here: http://jenkins.mxnet-ci.amazon-ml.com/blue/rest/organizations/jenkins/pipelines/incubator-mxnet/branches/master/runs/1601/nodes/102/log/?start=0
We've also added ASAN to the build environment. It can be used locally as well. I've written up a small guide for how to use ASAN in the wiki here: https://cwiki.apache.org/confluence/display/MXNET/Detecting+Memory+Leaks+and+Buffer+Overflows+in+MXNet?focusedCommentId=93323377#comment-93323377 . I'm going to gradually start modernizing the code-base, addressing ASAN issues, turning on clang tidy warnings (or errors with best judgement). I'd encourage other community members to keep their eyes open for issues they'd be interested in fixing. We've just had a release, so I'd suggest that now is a good time for refactors and style fixes. I've got a few open PRs already starting this work. I would appreciate it if a committer could give a review: https://github.com/pulls?q=is%3Aopen+is%3Apr+author%3AKellenSunderland+archived%3Afalse+label%3Apr-awaiting-review -Kellen On Mon, Aug 27, 2018 at 10:02 AM kellen sunderland < [email protected]> wrote: > Great suggestion Philip, and thanks for the reference. I've run a few > sanitizers locally but didn't see any output that concerned me. Absolutely > on my todo list to get them integrated into CI (unless someone beats me to > it). > > On Sun, Aug 26, 2018 at 1:54 AM Marco de Abreu > <[email protected]> wrote: > >> Great idea, thanks a lot Kellen. I had a look at the results and I really >> like that they are clearly actionable and reveal issues that somebody >> wouldn't have caught on the first sight. >> >> My personal favorite is the branch anomaly detection which finds execution >> paths that would result in a function returning garbage data if a certain >> path is being taken. I'm looking forward to more results! >> >> Best regards, >> Marco >> >> Philip Cho <[email protected]> schrieb am Sa., 25. Aug. 2018, >> 21:48: >> >> > +1 as well. >> > >> > As a related idea, let's also consider adding sanitizer tests, which >> > detects leaks and memory errors at runtime. Overhead is a lot lower than >> > other methods such as valgrind. >> > For an example, see the sanitizer tests in XGBoost: >> > https://github.com/dmlc/xgboost/pull/3557 >> > https://github.com/dmlc/xgboost/pull/3525 >> > >> > Hyunsu Cho. >> > >> > On Sat, Aug 25, 2018, 11:47 AM Hagay Lupesko <[email protected]> wrote: >> > >> > > I really like this proposal. >> > > It will help improve the quality of MXNet native code, and maintain a >> > > uniform high bar. >> > > An extra 5 mins of build time seems reasonable. >> > > >> > > +1 >> > > >> > > >> > > On Sat, Aug 25, 2018, 07:02 kellen sunderland < >> > [email protected] >> > > > >> > > wrote: >> > > >> > > > Hello all, >> > > > >> > > > Inspired by Vanadana, cclaus and the project members who setup the >> very >> > > > solid linting tools already in place for MXNet, I'm propose we >> enable >> > > > clang-tidy-6.0 in our CI (PR here: >> > > > https://github.com/apache/incubator-mxnet/pull/12282). clang-tidy >> is >> > > > getting to be quite a high-quality, free, easy-to-use static >> analysis >> > > tool >> > > > for modern C++. In my opinion it's a very useful extension of >> clang's >> > > > already great code warning system. Adding it to MXNet might help us >> > > catch >> > > > a few errors (memory leaks, use-after-frees, etc.) and it could >> help us >> > > > keep our coding standards uniform between contributors. It should >> also >> > > > help automate some of the work that is currently required of the PR >> > > > reviewers. >> > > > >> > > > I'd suggest we initially enabled clang-tidy as a mostly >> informational >> > CI >> > > > build step that will give many warnings, as in this sample run: >> > > > >> > > > >> > > >> > >> http://jenkins.mxnet-ci.amazon-ml.com/blue/rest/organizations/jenkins/pipelines/incubator-mxnet/branches/PR-12282/runs/20/nodes/102/log/?start=0 >> > > > (warnings at bottom of the build). We'll be able to optionally fail >> > the >> > > > build if certain rules are violated. There's a complete list of >> rules >> > > > here: >> > > > >> > > > >> > > >> > >> https://releases.llvm.org/6.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/list.html >> > > > . >> > > > If anyone has a controversial rule they'd like to see enforced, feel >> > free >> > > > to nominate rule in this thread. Non-controversial (use your best >> > > > judgement) rules can be enabled with a workflow similar to what we >> > > already >> > > > do with pylint. Choose a rule, make changes to the codebase such >> that >> > > that >> > > > rule will pass and add the rule to the .clang-tidy configuration >> file >> > in >> > > > the root of the repo. The current formatting of the file should >> make >> > it >> > > > obvious which rules are enabled as warnings/failures. >> > > > >> > > > I'd estimate once the PR is merged the build times for this task >> will >> > be >> > > > pretty nominal (4-5 minutes). Since the task is run in parallel, it >> > > should >> > > > have no impact on the PR total verification times. I also think >> that >> > > > introducing a tool like this right after a release has been cut >> will be >> > > > convenient for developers. It's a good time to introduce large >> fixup >> > > > changes, and it will give us lot of time to find any potential side >> > > effects >> > > > of modernization refactors. >> > > > >> > > > What does everyone think? Does it make sense to introduce >> clang-tidy >> > and >> > > > gradually address or enforce rules as with cpplint / pylint / >> flake8? >> > > > >> > > > -Kellen >> > > > >> > > >> > >> >
