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

UBSAN issues: RcppParallel #52

Closed
jlmelville opened this issue Feb 28, 2020 · 2 comments
Closed

UBSAN issues: RcppParallel #52

jlmelville opened this issue Feb 28, 2020 · 2 comments

Comments

@jlmelville
Copy link
Owner

Running rhub::check_with_sanitizers() has confirmed that the UBSAN issues reported for RcppAnnoy in #50 are fixed with RcppAnnoy 0.0.15. Unfortunately, there are lots of UBSAN complaints originating with RcppParallel. I don't think this is due to me using the package incorrectly, because the RcppParallel CRAN checks give the same messages (see https://www.stats.ox.ac.uk/pub/bdr/memtests/gcc-UBSAN/RcppParallel/RcppParallel-Ex.Rout).

They seem to originate with the Intel tbb library and are well known by the RcppParallel maintainers (see e.g. RcppCore/RcppParallel#36), but they can't do anything about it. The risk here is that the strategy of saying that the UBSAN issues are harmless and originate from a package uwot is using is exactly the strategy that stopped working with RcppAnnoy.

A possible alternative is to look at RcppThread which has a parallel for construct and is not currently showing any check problems.

@jlmelville
Copy link
Owner Author

I had hoped that setting -DRCPP_PARALLEL_USE_TBB=0 in the compiler flags would turn off the use of TBB and just use TinyThread++. Unfortunately, that doesn't seem to be the case. I have asked about this at RcppCore/RcppParallel#106.

At any rate, if I take the relevant bits of RcppParallel, add it to an inst directory and thoroughly scour it of any tbb-related material, then the sanitizer seems to pass (it claims there is a 'prep error', but I don't see any runtime errors in the output).

If there is any good news to doing things this way, it's that we don't need a Makevars.win, we don't need to go to RScript to get the linker path and we don't need to use the non-standard += in Makevars, which lets us avoid the NOTE in the check (maybe we never needed it though).

If I have to go down this road, perhaps maintenance would be simpler (at the cost of more up-front work) by keeping only what is necessary for the parallel-for code, adapting it for std::thread, and dropping even tthread.

jlmelville added a commit that referenced this issue Mar 1, 2020
#52: Use RcppParallel subset directly
@jlmelville
Copy link
Owner Author

There's a new version of RcppParallel out that seems to fix these issues, but I have stuck with a std::thread-only version of the code I need which is less than 80 lines. It's in inst/include/RcppPerpendicular.h (renamed to avoid any clashing of namespaces but to indicate its origin) and remains GPL2.

yuhanH pushed a commit to yuhanH/uwot that referenced this issue Jul 20, 2020
yuhanH pushed a commit to yuhanH/uwot that referenced this issue Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant