Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CMake option to enable sanitizers and build gtest #3555

Merged
merged 7 commits into from
Mar 12, 2021

Conversation

hcho3
Copy link
Contributor

@hcho3 hcho3 commented Nov 11, 2020

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
Copy link

ghost commented Nov 11, 2020

CLA assistant check
All CLA requirements met.

@hcho3 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 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
@AlbertoEAF
Copy link
Contributor

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
Copy link
Contributor Author

hcho3 commented Feb 17, 2021

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

@AlbertoEAF
Copy link
Contributor

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
Copy link
Collaborator

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 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 hcho3 marked this pull request as ready for review February 18, 2021 01:26
@hcho3
Copy link
Contributor Author

hcho3 commented Feb 18, 2021

Marking it ready for review.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
cmake/Sanitizer.cmake Outdated Show resolved Hide resolved
cmake/Sanitizer.cmake Outdated Show resolved Hide resolved
cmake/Sanitizer.cmake Outdated Show resolved Hide resolved
cmake/modules/FindASan.cmake Outdated Show resolved Hide resolved
cmake/modules/FindASan.cmake Outdated Show resolved Hide resolved
cmake/modules/FindUBSan.cmake Outdated Show resolved Hide resolved
tests/cpp_test/test_main.cpp Outdated Show resolved Hide resolved
@hcho3
Copy link
Contributor Author

hcho3 commented Feb 20, 2021

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

@StrikerRUS
Copy link
Collaborator

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

image

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

cc @jameslamb

cmake/Sanitizer.cmake Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
@@ -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")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
Co-authored-by: Nikita Titov <[email protected]>
@shiyu1994
Copy link
Collaborator

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
Copy link
Collaborator

@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 🙁 .

@hcho3 hcho3 requested a review from shiyu1994 as a code owner March 12, 2021 00:34
@StrikerRUS StrikerRUS merged commit bcf443b into microsoft:master Mar 12, 2021
@hcho3 hcho3 deleted the add_sanitizer branch March 12, 2021 21:58
@jameslamb
Copy link
Collaborator

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

StrikerRUS added a commit that referenced this pull request Mar 25, 2021
* [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
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants