Skip to content

Remove unprotect calls that rchk claims to be over unprotecting. #456

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

Closed
wants to merge 3 commits into from

Conversation

astamm
Copy link

@astamm astamm commented May 7, 2025

While checking against rhub/rchk the {phutil} package I am about to submit to CRAN, it spotted:

Function cpp11::detail::store::insert(SEXPREC*)
  [PB] has negative depth /github/home/R/x86_64-pc-linux-gnu-library/4.6/cpp11/include/cpp11/protect.hpp:302
  [UP] attempt to unprotect more items (2) than protected (0), results will be incomplete /github/home/R/x86_64-pc-linux-gnu-library/4.6/cpp11/include/cpp11/protect.hpp:302
  [PB] has possible protection stack imbalance /github/home/R/x86_64-pc-linux-gnu-library/4.6/cpp11/include/cpp11/protect.hpp:305

Function cpp11::writable::r_vector<double>::reserve_data(SEXPREC*, bool, long)
  [PB] has negative depth /github/home/R/x86_64-pc-linux-gnu-library/4.6/cpp11/include/cpp11/r_vector.hpp:1342
  [UP] attempt to unprotect more items (2) than protected (0), results will be incomplete /github/home/R/x86_64-pc-linux-gnu-library/4.6/cpp11/include/cpp11/r_vector.hpp:1342
  [PB] has possible protection stack imbalance /github/home/R/x86_64-pc-linux-gnu-library/4.6/cpp11/include/cpp11/r_vector.hpp:1343

Function cpp11::writable::r_vector<double>::resize_data(SEXPREC*, bool, long)
  [PB] has negative depth /github/home/R/x86_64-pc-linux-gnu-library/4.6/cpp11/include/cpp11/r_vector.hpp:1367
  [UP] attempt to unprotect more items (1) than protected (0), results will be incomplete /github/home/R/x86_64-pc-linux-gnu-library/4.6/cpp11/include/cpp11/r_vector.hpp:1367
  [PB] has possible protection stack imbalance /github/home/R/x86_64-pc-linux-gnu-library/4.6/cpp11/include/cpp11/r_vector.hpp:1368

Function cpp11::writable::r_vector<double>::resize_names(SEXPREC*, long)
  [PB] has negative depth /github/home/R/x86_64-pc-linux-gnu-library/4.6/cpp11/include/cpp11/r_vector.hpp:1389
  [UP] attempt to unprotect more items (1) than protected (0), results will be incomplete /github/home/R/x86_64-pc-linux-gnu-library/4.6/cpp11/include/cpp11/r_vector.hpp:1389

Full details here: https://github.com/tdaverse/phutil/actions/runs/14893316603/job/41830462913.

I therefore temporarily vendored {cpp11} headers and removed the UNPROTECT() calls and now rchk does not complain anymore.

This PR proposes their removal but maybe it needs more tests/investigation.

@astamm
Copy link
Author

astamm commented May 7, 2025

Though it seems to break the CI because FAQ vignette fails with these modifications.
Maybe it was a false positive from rchk.
If you can shed some light on this, I would very much appreciate it. Thanks.

@DavisVaughan
Copy link
Member

These unprotects look correct to me. You can see the matching PROTECT() above them.

@astamm
Copy link
Author

astamm commented May 9, 2025

Yes, but still rchk complains.

I found a fix for insert in protect.hpp.

The key changes were to remove PROTECT/UNPROTECT calls since:

  • list from get() is already preserved;
  • The cons cell is part of the preserve list structure;
  • The input x is protected by the caller.

I have not yet managed to find the issue in r_vector.hpp.

@astamm
Copy link
Author

astamm commented May 9, 2025

I found the fix for r_vector.hpp. In summary,

Key changes to reserve_data()

  1. Removed all explicit PROTECT/UNPROTECT calls since protection is handled by the safe wrapper in both resize_data and resize_names()
  2. Simplified the flow of the names handling
  3. Let R's garbage collection handle the intermediate objects

Key change to resize_data()

  1. Moving both the PROTECT and UNPROTECT calls since the safe wrapper around Rf_allocVector already handles protection.

Key changes to resize_names()

  1. The safe wrapper around Rf_allocVector already handles protection
  2. Removed direct pointer access via STRING_PTR_RO
  3. Use STRING_ELT instead of direct pointer access

@DavisVaughan
Copy link
Member

The safe wrapper does not handle protection when the output type is SEXP. The caller must still protect the SEXP output. There must be a problem with rchk, possibly only with r-devel, the existing code looks fine to me.

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

Successfully merging this pull request may close these issues.

2 participants