Solution to [Rcpp-devel] Runtime error from Rcpp::depends with gcc sanitizers

[boennecd]

I ran into the exact issue described in this post. As I could not figure out whether this has been solved, then I post a solution here. The cause of the problem seems to be uninitialized bools. This is easy to confirm. First, consider the setup in the mailing list:

docker run -it --rm rhub/rocker-gcc-san bash
Rdevel -e "install.packages('remotes')"
Rdevel -e "remotes::install_github('boennecd/Rcpp', ref = 'c5a97a3')"
Rdevel -e "Rcpp::sourceCpp(code = '
#include <Rcpp.h>
// [[Rcpp::depends(Matrix)]]
// [[Rcpp::export]]
int foo() { return 0; }')"

This still yields:

attributes.cpp:169:11: runtime error: load of value 2, which is not a valid value for type 'bool'

The error is removed when one initalize the private members as I have done in the repository I install here:

Rdevel -e "remotes::install_github('boennecd/Rcpp', ref = 'abff123')"
Rdevel -e "Rcpp::sourceCpp(code = '
#include <Rcpp.h>
// [[Rcpp::depends(Matrix)]]
// [[Rcpp::export]]
int foo() { return 0; }')"

There is no error in this case.

[eddelbuettel]

Glancing at your commit makes it look innocent enough. I had to expand a little to see the context -- false seems like the right approach.

I happen to be running a full rev.dep for Rcpp right now anyway. If you want to PR this, I could test it and @jjallaire can and will take a look too as it his code (and, without putting words in his mouth, likely bless it).

Nice catch. Appreciate the digging.

[boennecd]

Nice catch. Appreciate the digging.

Glad to help! I figured the default should be false.

If you want to PR this, I could test it ...

The question is whether I should make sure to initialize all the other (private) bool members in src/attributes.cpp?

[jjallaire]

Yes, that fix is absolutely correct! One qualifier: previously these values were essentially undefined, so likely evaluated to true (which would cause generated code to have const/reference semantics). This could in theory therefore cause changes in attributes output. My belief is that in-fact we aren't using the default constructor anywhere (except possibly to make it's inclusion in stl collections possible) so no change in behavior will occur. Probably worth poking a bit at though.

We don't need to initialize other private members unless they are primitive types (e.g. std::string, etc. already have default initializations).

[eddelbuettel]

There two or three other private bool variables in the file. Should they all be false? Can you take a really quick glance?

[jjallaire]

Had a look and they all look to be properly initialized in the respective constructors. Looking further at the original case, I think the empty constructor is just there to facilitate the `empty()` method, and that no "real" Type instance would suffer from the uninitialized values.

…

On Mon, Oct 19, 2020 at 2:09 PM Dirk Eddelbuettel ***@***.***> wrote: There two or three other private bool variables in the file. Should they all be false? Can you take a really quick glance? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1112 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZPR66FQYQDFGGS76TY4DSLR6FLANCNFSM4SWM7WMA> .

[boennecd]

Had a look and they all look to be properly initialized in the respective
constructors. Looking further at the original case, I think the
empty constructor is just there to facilitate the empty() method, and
that no "real" Type instance would suffer from the uninitialized values.

Thank you for looking into this! While there may be no effect of the uninitialized values, there still seems to be the issue with some sanitizers. Would it not be nice to avoid? I spend 2 hours on this as I thought there was an issue with my code.

If you agree, do you want me to create a pull request?

[boennecd]

I am sorry if I misunderstood your point.

[eddelbuettel]

I read JJ's comment as stating that other constructors appear to be fine. We both think that what you found on the initial two you changed is a very good and helpful fix. Please prepare a PR for that.

[jjallaire]

Yes, my guess is that sanitizers won't complain any longer after your PR. If they do by all means feel free to give us another PR!

…

On Tue, Oct 20, 2020 at 11:24 AM Dirk Eddelbuettel ***@***.***> wrote: I read JJ's comment as stating that *other* constructors appear to be fine. We both think that what you found on the initial two you changed is a very good and helpful fix. Please prepare a PR for that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1112 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZPR3JH7SO3LFZVRXURQDSLWTSBANCNFSM4SWM7WMA> .

[eddelbuettel]

Now fixed.