Add CMake option to enable sanitizers and build gtest #3555

hcho3 · 2020-11-11T09:22:22Z

Following up with #3338 (comment). Quote:

My suggestion is to use Address and Leak Sanitizers. The sanitizers add extra instrumentation code to detect bad memory accesses and leaks at runtime, and unlike Valgrind have relatively light impact on performance.
XGBoost's CI system runs C++ tests with sanitizers enabled, and so far it has caught a few memory issues (example: dmlc/xgboost#3557). I highly recommend it.

TODOs

Add CMake option to enable sanitizers (address, leak, undefined, thread)
Add CMake option to link with gtest (Google C++ testing framework)
Add C++ tests. The goal is to produce a standalone testing executable that can be run with sanitizers.

Why gtest? In my experience, a shared library linked with AddressSanitizer cannot be loaded into Python or R session. It results in error

==48458==ASan runtime does not come first in initial library list; you should either link runtime to your application or manually preload it with LD_PRELOAD.

The most convenient setup is to build a standalone executable with sanitizers enabled. An additional benefit is that the C++ test executable tends to run quite fast, even when sanitizers are enabled.

An example: https://xgboost-ci.net/blue/organizations/jenkins/xgboost/detail/master/604/pipeline/57#step-122-log-1, which ran the suite of C++ tests hosted in this directory. The suite completed in about one minute.

ghost · 2020-11-11T09:22:34Z

All CLA requirements met.

AlbertoEAF · 2021-02-17T14:55:11Z

Hi there, what's the state of this PR? @hcho3

I'd like to start adding C++ tests too with gtest in another MR, so it would be immensely useful to have this merged already :)

hcho3 · 2021-02-17T15:08:20Z

I have not gotten around adding actual test cases in gtest. Right now this PR only adds gtest as an optional CMake target.

AlbertoEAF · 2021-02-17T15:14:01Z

Not sure how others feel about it, but I think we can add tests in a separate MR and add support for gtest already so other MR's can start using gtest ;)

@StrikerRUS ?

StrikerRUS · 2021-02-18T00:52:11Z

Not sure how others feel about it, but I think we can add tests in a separate MR and add support for gtest already so other MR's can start using gtest ;)

I support this idea!
Let's merge PR with infrastructure setup and then everyone will be able to add tests.

@hcho3 Is this PR completed from your side and ready for review excluding "Add C++ tests." from TODOs?

hcho3 · 2021-02-18T01:26:38Z

Marking it ready for review.

StrikerRUS

@hcho3 Thank you very much for the awesome contribution! Please consider checking some my comments/questions and suggestions below.

CMakeLists.txt

cmake/Sanitizer.cmake

cmake/modules/FindASan.cmake

cmake/modules/FindUBSan.cmake

tests/cpp_test/test_main.cpp

hcho3 · 2021-02-20T14:17:41Z

I'm on vacation until March 3. I will revisit this PR when I am back.

StrikerRUS · 2021-03-07T20:34:39Z

@hcho3 Could you please sync your branch with current master?

I'm not sure but guess this is the reason of failing Dask tests.

cc @jameslamb

cmake/Sanitizer.cmake

StrikerRUS · 2021-03-07T20:43:17Z

CMakeLists.txt

@@ -6,6 +6,12 @@ OPTION(USE_HDFS "Enable HDFS support (EXPERIMENTAL)" OFF)
 OPTION(USE_TIMETAG "Set to ON to output time costs" OFF)
 OPTION(USE_CUDA "Enable CUDA-accelerated training (EXPERIMENTAL)" OFF)
 OPTION(USE_DEBUG "Set to ON for Debug mode" OFF)
+OPTION(USE_SANITIZER "Use santizer flags" OFF)
+OPTION(SANITIZER_PATH "Path to sanitizer libs")


Based on #3555 (comment), this should be SET, right?

StrikerRUS

Many thanks for the great help!
LGTM for CMake part except one minor suggestion below.

Someone please check cpp part. Maybe @shiyu1994 @btrotta ?

CMakeLists.txt

Co-authored-by: Nikita Titov <[email protected]>

shiyu1994 · 2021-03-11T01:53:05Z

Tested the BUILD_CPP_TEST option. The output is just fine, except that there's no actual tests. But as mentioned above, adding cpp tests will be a future work.

Thanks @hcho3 .

StrikerRUS · 2021-03-12T00:28:48Z

@hcho3 Sorry for the inconvenience, but could you please sync the branch one more time, so that tests will pass and we will be able to merge the PR? We had a lot of CI issues this and past week 🙁 .

jameslamb · 2021-03-12T22:12:34Z

Thank you so much for the help @hcho3 ! Awesome addition.

* [docs]Add alt text on images * Update docs/GPU-Windows.rst Co-authored-by: James Lamb <[email protected]> * Update docs/GPU-Windows.rst Co-authored-by: James Lamb <[email protected]> * Apply suggestions from code review Co-authored-by: James Lamb <[email protected]> * Apply suggestions from code review Co-authored-by: James Lamb <[email protected]> * Merge main branch commit updates (#1) * [docs] Add alt text to image in Parameters-Tuning.rst (#4035) * [docs] Add alt text to image in Parameters-Tuning.rst Add alt text to Leaf-wise growth image, as part of #4028 * Update docs/Parameters-Tuning.rst Co-authored-by: James Lamb <[email protected]> Co-authored-by: James Lamb <[email protected]> * [ci] [R-package] upgrade to R 4.0.4 in CI (#4042) * [docs] update description of deterministic parameter (#4027) * update description of deterministic parameter to require using with force_row_wise or force_col_wise * Update include/LightGBM/config.h Co-authored-by: Nikita Titov <[email protected]> * update docs Co-authored-by: Nikita Titov <[email protected]> * [dask] Include support for init_score (#3950) * include support for init_score * use dataframe from init_score and test difference with and without init_score in local model * revert refactoring * initial docs. test between distributed models with and without init_score * remove ranker from tests * test value for root node and change docs * comma * re-include parametrize * fix incorrect merge * use single init_score and the booster_ attribute * use np.float64 instead of float * [ci] ignore untitle Jupyter notebooks in .gitignore (#4047) * [ci] prevent getting incompatible dask and distributed versions (#4054) * [ci] prevent getting incompatible dask and distributed versions * Update .ci/test.sh Co-authored-by: Nikita Titov <[email protected]> * empty commit Co-authored-by: Nikita Titov <[email protected]> * [ci] fix R CMD CHECK note about example timings (fixes #4049) (#4055) * [ci] fix R CMD CHECK note about example timings (fixes #4049) * Apply suggestions from code review Co-authored-by: Nikita Titov <[email protected]> * empty commit Co-authored-by: Nikita Titov <[email protected]> * [ci] add CMake + R 3.6 test back (fixes #3469) (#4053) * [ci] add CMake + R 3.6 test back (fixes #3469) * Apply suggestions from code review Co-authored-by: Nikita Titov <[email protected]> * Update .ci/test_r_package_windows.ps1 * -Wait and remove rtools40 * empty commit Co-authored-by: Nikita Titov <[email protected]> * [dask] include multiclass-classification task in tests (#4048) * include multiclass-classification task and task_to_model_factory dicts * define centers coordinates. flatten init_scores within each partition for multiclass-classification * include issue comment and fix linting error * Update index.rst (#4029) Add alt text to logo image Co-authored-by: James Lamb <[email protected]> * [dask] raise more informative error for duplicates in 'machines' (fixes #4057) (#4059) * [dask] raise more informative error for duplicates in 'machines' * uncomment * avoid test failure * Revert "avoid test failure" This reverts commit 9442bdf. * [dask] add tutorial documentation (fixes #3814, fixes #3838) (#4030) * [dask] add tutorial documentation (fixes #3814, fixes #3838) * add notes on saving the model * quick start examples * add examples * fix timeouts in examples * remove notebook * fill out prediction section * table of contents * add line back * linting * isort * Apply suggestions from code review Co-authored-by: Nikita Titov <[email protected]> * Apply suggestions from code review Co-authored-by: Nikita Titov <[email protected]> * move examples under python-guide * remove unused pickle import Co-authored-by: Nikita Titov <[email protected]> * set 'pending' commit status for R Solaris optional workflow (#4061) * [docs] add Yu Shi to repo maintainers (#4060) * Update FAQ.rst * Update CODEOWNERS * set is_linear_ to false when it is absent from the model file (fix #3778) (#4056) * Add CMake option to enable sanitizers and build gtest (#3555) * Add CMake option to enable sanitizer * Set up gtest * Address reviewer's feedback * Address reviewer's feedback * Update CMakeLists.txt Co-authored-by: Nikita Titov <[email protected]> Co-authored-by: Nikita Titov <[email protected]> * added type hint (#4070) * [ci] run Dask examples on CI (#4064) * Update Parallel-Learning-Guide.rst * Update test.sh * fix path * address review comments * [python-package] add type hints on Booster.set_network() (#4068) * [python-package] add type hints on Booster.set_network() * change behavior * [python-package] Some mypy fixes (#3916) * Some mypy fixes * address James' comments * Re-introduce pass in empty classes * Update compat.py Remove extra lines * [dask] [ci] fix flaky network-setup test (#4071) * [tests][dask] simplify code in Dask tests (#4075) * simplify Dask tests code * enable CI * disable CI * Revert "[ci] prevent getting incompatible dask and distributed versions (#4054)" (#4076) This reverts commit 4e9c976. * Fix parsing of non-finite values (#3942) * Fix index out-of-range exception generated by BaggingHelper on small datasets. Prior to this change, the line "score_t threshold = tmp_gradients[top_k - 1];" would generate an exception, since tmp_gradients would be empty when the cnt input value to the function is zero. * Update goss.hpp * Update goss.hpp * Add API method LGBM_BoosterPredictForMats which runs prediction on a data set given as of array of pointers to rows (as opposed to existing method LGBM_BoosterPredictForMat which requires data given as contiguous array) * Fix incorrect upstream merge * Add link to LightGBM.NET * Fix indenting to 2 spaces * Dummy edit to trigger CI * Dummy edit to trigger CI * remove duplicate functions from merge * Fix parsing of non-finite values. Current implementation silently returns zero when input string is "inf", "-inf", or "nan" when compiled with VS2017, so instead just explicitly check for these values and fail if there is no match. No attempt to optimise string allocations in this implementation since it is usually rarely invoked. * Dummy commit to trigger CI * Also handle -nan in double parsing method * Update include/LightGBM/utils/common.h Remove trailing whitespace to pass linting tests Co-authored-by: Nikita Titov <[email protected]> Co-authored-by: matthew-peacock <[email protected]> Co-authored-by: Guolin Ke <[email protected]> Co-authored-by: Nikita Titov <[email protected]> * [dask] remove unused imports from typing (#4079) * Range check for DCG position discount lookup (#4069) * Add check to prevent out of index lookup in the position discount table. Add debug logging to report number of queries found in the data. * Change debug logging location so that we can print the data file name as well. * Revert "Change debug logging location so that we can print the data file name as well." This reverts commit 3981b34. * Add data file name to debug logging. * Move log line to a place where it is output even when query IDs are read from a separate file. * Also add the out-of-range check to rank metrics. * Perform check after number of queries is initialized. * Update * [ci] upgrade R CI scripts to work on Ubuntu 20.04 (#4084) * [ci] install additional LaTeX packages in R CI jobs * update autoconf version * bump upper limit on package size to 100 * [SWIG] Add streaming data support + cpp tests (#3997) * [feature] Add ChunkedArray to SWIG * Add ChunkedArray * Add ChunkedArray_API_extensions.i * Add SWIG class wrappers * Address some review comments * Fix linting issues * Move test to tests/test_ChunkedArray_manually.cpp * Add test note * Move ChunkedArray to include/LightGBM/utils/ * Declare more explicit types of ChunkedArray in the SWIG API. * Port ChunkedArray tests to googletest * Please C++ linter * Address StrikerRUS' review comments * Update SWIG doc & disable ChunkedArray<int64_t> * Use CHECK_EQ instead of assert * Change include order (linting) * Rename ChunkedArray -> chunked_array files * Change header guards * Address last comments from StrikerRUS * store all CMake files in one place (#4087) * v3.2.0 release (#3872) * Update VERSION.txt * update appveyor.yml and configure * fix Appveyor builds Co-authored-by: James Lamb <[email protected]> Co-authored-by: Nikita Titov <[email protected]> Co-authored-by: StrikerRUS <[email protected]> * [ci] Bump version for development (#4094) * Update .appveyor.yml * Update cran-comments.md * Update VERSION.txt * update configure Co-authored-by: James Lamb <[email protected]> * [ci] fix flaky Azure Pipelines jobs (#4095) * Update test.sh * Update setup.sh * Update .vsts-ci.yml * Update test.sh * Update setup.sh * Update .vsts-ci.yml * Update setup.sh * Update setup.sh Co-authored-by: Subham Agrawal <[email protected]> Co-authored-by: James Lamb <[email protected]> Co-authored-by: shiyu1994 <[email protected]> Co-authored-by: Nikita Titov <[email protected]> Co-authored-by: jmoralez <[email protected]> Co-authored-by: marcelonieva7 <[email protected]> Co-authored-by: Philip Hyunsu Cho <[email protected]> Co-authored-by: Deddy Jobson <[email protected]> Co-authored-by: Alberto Ferreira <[email protected]> Co-authored-by: mjmckp <[email protected]> Co-authored-by: matthew-peacock <[email protected]> Co-authored-by: Guolin Ke <[email protected]> Co-authored-by: ashok-ponnuswami-msft <[email protected]> Co-authored-by: StrikerRUS <[email protected]> * Apply suggestions from code review Co-authored-by: Nikita Titov <[email protected]> Co-authored-by: James Lamb <[email protected]> Co-authored-by: Subham Agrawal <[email protected]> Co-authored-by: shiyu1994 <[email protected]> Co-authored-by: Nikita Titov <[email protected]> Co-authored-by: jmoralez <[email protected]> Co-authored-by: marcelonieva7 <[email protected]> Co-authored-by: Philip Hyunsu Cho <[email protected]> Co-authored-by: Deddy Jobson <[email protected]> Co-authored-by: Alberto Ferreira <[email protected]> Co-authored-by: mjmckp <[email protected]> Co-authored-by: matthew-peacock <[email protected]> Co-authored-by: Guolin Ke <[email protected]> Co-authored-by: ashok-ponnuswami-msft <[email protected]> Co-authored-by: StrikerRUS <[email protected]>

github-actions · 2023-08-23T23:58:41Z

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

Add CMake option to enable sanitizer

a8430ae

hcho3 changed the title ~~[WIP] Add CMake option to enable sanitizer + Add C++ tests with gtest~~ [WIP] Add CMake option to enable sanitizer + Enable C++ tests with gtest Nov 11, 2020

hcho3 changed the title ~~[WIP] Add CMake option to enable sanitizer + Enable C++ tests with gtest~~ [WIP] Add CMake option to enable sanitizers + Enable C++ tests with gtest Nov 11, 2020

Set up gtest

77422b2

hcho3 force-pushed the add_sanitizer branch from 5ca3f20 to 77422b2 Compare November 11, 2020 09:43

jameslamb added the maintenance label Nov 21, 2020

StrikerRUS mentioned this pull request Feb 17, 2021

[feature] Streaming data allocation #3995

Closed

AlbertoEAF mentioned this pull request Feb 17, 2021

[SWIG] Add streaming data support + cpp tests #3997

Merged

hcho3 changed the title ~~[WIP] Add CMake option to enable sanitizers + Enable C++ tests with gtest~~ Add CMake option to enable sanitizers and build gtest Feb 18, 2021

hcho3 marked this pull request as ready for review February 18, 2021 01:26

hcho3 requested review from btrotta, chivee, guolinke, henry0312, huanzhang12, jameslamb, Laurae2, StrikerRUS and wxchan as code owners February 18, 2021 01:26

StrikerRUS mentioned this pull request Feb 18, 2021

Fix parsing of non-finite values #3942

Merged

StrikerRUS reviewed Feb 19, 2021

View reviewed changes

Address reviewer's feedback

eb3eea0

StrikerRUS reviewed Mar 7, 2021

View reviewed changes

hcho3 added 2 commits March 8, 2021 15:52

Merge remote-tracking branch 'upstream/master' into add_sanitizer

92952a5

Address reviewer's feedback

dfaa89e

StrikerRUS approved these changes Mar 9, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Update CMakeLists.txt

433ea47

Co-authored-by: Nikita Titov <[email protected]>

Merge remote-tracking branch 'upstream/master' into add_sanitizer

d3239c6

hcho3 requested a review from shiyu1994 as a code owner March 12, 2021 00:34

StrikerRUS merged commit bcf443b into microsoft:master Mar 12, 2021

hcho3 deleted the add_sanitizer branch March 12, 2021 21:58

StrikerRUS mentioned this pull request Mar 27, 2021

Refactor CMakeLists.txt so that it will be possible to build cpp tests with different options, e.g. with OpenMP support #4125

Closed

StrikerRUS mentioned this pull request Apr 6, 2021

[ci] run cpp tests at CI #4166

Merged

StrikerRUS mentioned this pull request May 15, 2021

[tests][cli] distributed training #4254

Merged

StrikerRUS mentioned this pull request Jun 9, 2021

[docs] document sanitizers #4365

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CMake option to enable sanitizers and build gtest #3555

Add CMake option to enable sanitizers and build gtest #3555

hcho3 commented Nov 11, 2020 •

edited

Loading

ghost commented Nov 11, 2020 •

edited by ghost

Loading

AlbertoEAF commented Feb 17, 2021

hcho3 commented Feb 17, 2021 •

edited

Loading

AlbertoEAF commented Feb 17, 2021

StrikerRUS commented Feb 18, 2021

hcho3 commented Feb 18, 2021

StrikerRUS left a comment

hcho3 commented Feb 20, 2021

StrikerRUS commented Mar 7, 2021

StrikerRUS Mar 7, 2021

StrikerRUS left a comment

shiyu1994 commented Mar 11, 2021

StrikerRUS commented Mar 12, 2021

jameslamb commented Mar 12, 2021

github-actions bot commented Aug 23, 2023

Add CMake option to enable sanitizers and build gtest #3555

Add CMake option to enable sanitizers and build gtest #3555

Conversation

hcho3 commented Nov 11, 2020 • edited Loading

ghost commented Nov 11, 2020 • edited by ghost Loading

AlbertoEAF commented Feb 17, 2021

hcho3 commented Feb 17, 2021 • edited Loading

AlbertoEAF commented Feb 17, 2021

StrikerRUS commented Feb 18, 2021

hcho3 commented Feb 18, 2021

StrikerRUS left a comment

Choose a reason for hiding this comment

hcho3 commented Feb 20, 2021

StrikerRUS commented Mar 7, 2021

StrikerRUS Mar 7, 2021

Choose a reason for hiding this comment

StrikerRUS left a comment

Choose a reason for hiding this comment

shiyu1994 commented Mar 11, 2021

StrikerRUS commented Mar 12, 2021

jameslamb commented Mar 12, 2021

github-actions bot commented Aug 23, 2023

hcho3 commented Nov 11, 2020 •

edited

Loading

ghost commented Nov 11, 2020 •

edited by ghost

Loading

hcho3 commented Feb 17, 2021 •

edited

Loading