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.

Sign up for GitHub

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

Jump to bottom

PROTECT() as= input to coerceAs() #6249

Merged
merged 5 commits into from
Jul 13, 2024
Merged

PROTECT() as= input to coerceAs() #6249

merged 5 commits into from
Jul 13, 2024

Conversation

MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Jul 12, 2024 •
edited
Loading

As identified by rchk on CRAN:

https://raw.githubusercontent.com/kalibera/cran-checks/master/rchk/results/data.table.out

Function coerceToRealListR
  [UP] calling allocating function coerceAs(?,V,?) with argument allocated using Rf_ScalarReal data.table/src/frollR.c:18

Function frollapplyR
  [UP] calling allocating function coerceAs(?,V,?) with argument allocated using Rf_ScalarReal data.table/src/frollR.c:260

Function frollfunR
  [UP] calling allocating function coerceAs(?,V,?) with argument allocated using Rf_ScalarReal data.table/src/frollR.c:148

Function gmean
  [UP] calling allocating function coerceAs(?,V,?) with argument allocated using Rf_ScalarReal data.table/src/gsumm.c:594

I'm a bit surprised about how conservative we need to be here, but I do see R's own sources PROTECT()ing the output of Scalar*:

https://github.com/search?q=repo%3Ar-devel%2Fr-svn%20%2FPROTECT%5B(%5D(Rf_)%3FScalar%2F&type=code

I don't understand well why the as= argument in particular needs to be protected here...

@MichaelChirico MichaelChirico changed the title PROTECT() Scalar* objects PROTECT() as= input to coerceAs() Jul 12, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 12, 2024 •
edited
Loading

Comparison Plot

Generated via commit 4b7a1f3

Download link for the artifact containing the test results: ↓ atime-results.zip

Time taken to finish the standard R installation steps: 12 minutes and 8 seconds

Time taken to run atime::atime_pkg on the tests: 3 minutes and 29 seconds

@ben-schwen
Copy link
Member

ben-schwen commented Jul 13, 2024 •
edited
Loading

Seems okay, but why don't we have to protect the copyArg then?

From WRE

Caller protection. It is the responsibility of the caller that all arguments passed to a function are protected and will stay protected for the whole execution of the callee. Typically this is achieved by PROTECT and UNPROTECT calls.

More clear guidelines for PROTECT are here

@MichaelChirico
Copy link
Member Author

Yea, I'm not sure. I did so in an earlier commit but backed off since it was a bit uglier.

Maybe it's an rchk thing that only the first problematic argument is caught? Let's try running {rchk} ourselves after this is merged.

@MichaelChirico MichaelChirico merged commit ad66a0d into master Jul 13, 2024
4 checks passed
@MichaelChirico MichaelChirico deleted the rchk-scalar branch July 13, 2024 17:33
@ben-schwen
Copy link
Member

ben-schwen commented Jul 13, 2024 •
edited
Loading

I just ran rchk locally and get

Function gmean
  [UP] allocating function gather may destroy its unprotected argument (x <arg 1>), which is later used. /rchk/packages/build/OFS5pfVE/data.table/src/gsumm.c:597
  [UP] calling allocating function gather with a fresh pointer (x <arg 1>) /rchk/packages/build/OFS5pfVE/data.table/src/gsumm.c:597
  [UP] unprotected variable x while calling allocating function Rf_allocVector /rchk/packages/build/OFS5pfVE/data.table/src/gsumm.c:598

Function rbindlist
  [UP] unprotected variable ans while calling allocating function Rf_getAttrib(?,S:names) /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:288
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_getAttrib(?,S:names) /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:288
  [UP] unprotected variable ans while calling allocating function R_compute_identical /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:312
  [UP] unprotected variable coercedForFactor while calling allocating function R_compute_identical /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:312
  [UP] unprotected variable ans while calling allocating function Rf_mkChar /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:322
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_mkChar /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:322
  [UP] unprotected variable ans while calling allocating function Rf_allocVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:327
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_allocVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:327
  [UP] unprotected variable ans while calling allocating function Rf_copyMostAttrib /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:328
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_copyMostAttrib /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:328
  [UP] unprotected variable ans while calling allocating function Rf_allocVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:334
  [UP] unprotected variable ans while calling allocating function Rf_coerceVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:341
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_coerceVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:341
  [UP] unprotected variable target while calling allocating function Rf_coerceVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:341
  [UP] unprotected variable ans while calling allocating function Rf_warning /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:495
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_warning /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:495
  [UP] unprotected variable target while calling allocating function Rf_warning /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:495
  [UP] unprotected variable ans while calling allocating function Rf_allocVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:497
  [UP] unprotected variable ans while calling allocating function Rf_setAttrib(?,S:levels,V) /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:497
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_allocVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:497
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_setAttrib(?,S:levels,V) /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:497
  [UP] unprotected variable target while calling allocating function Rf_allocVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:497
  [UP] unprotected variable ans while calling allocating function Rf_allocVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:502
  [UP] unprotected variable ans while calling allocating function Rf_setAttrib(?,S:class,V) /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:502
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_allocVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:502
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_setAttrib(?,S:class,V) /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:502
  [UP] unprotected variable ans while calling allocating function Rf_ScalarString /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:506
  [UP] unprotected variable ans while calling allocating function Rf_setAttrib(?,S:class,V) /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:506
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_ScalarString /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:506
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_setAttrib(?,S:class,V) /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:506
  [UP] unprotected variable ans while calling allocating function Rf_coerceVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:521
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_coerceVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:521
  [UP] unprotected variable target while calling allocating function Rf_coerceVector /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:521
  [UP] allocating function memrecycle may destroy its unprotected argument (target <arg 1>), which is later used. /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:524
  [UP] calling allocating function memrecycle with a fresh pointer (target <arg 1>) /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:524
  [UP] unprotected variable ans while calling allocating function memrecycle /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:524
  [UP] unprotected variable coercedForFactor while calling allocating function memrecycle /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:524
  [PB] has negative depth /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:525
  [UP] attempt to unprotect more items (1) than protected (0), results will be incomplete /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:525
  [UP] unprotected variable ans while calling allocating function Rf_warning /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:526
  [UP] unprotected variable coercedForFactor while calling allocating function Rf_warning /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:526
  [UP] unprotected variable target while calling allocating function Rf_warning /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:526
  [PB] has negative depth /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:534
  [UP] attempt to unprotect more items (1) than protected (0), results will be incomplete /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:534
  [UP] attempt to unprotect more items (2) than protected (0), results will be incomplete /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:534
  [UP] attempt to unprotect more items (2) than protected (1), results will be incomplete /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:534
  [PB] has possible protection stack imbalance /rchk/packages/build/OFS5pfVE/data.table/src/rbindlist.c:536

Function shallow
  [UP] unprotected variable newdt while calling allocating function Rf_shallow_duplicate /rchk/packages/build/OFS5pfVE/data.table/src/assign.c:168
  [UP] unprotected variable newdt while calling allocating function Rf_duplicate /rchk/packages/build/OFS5pfVE/data.table/src/assign.c:171
  [UP] unprotected variable newdt while calling allocating function Rf_getAttrib(?,S:names) /rchk/packages/build/OFS5pfVE/data.table/src/assign.c:173
  [UP] unprotected variable newdt while calling allocating function Rf_allocVector /rchk/packages/build/OFS5pfVE/data.table/src/assign.c:174
  [UP] allocating function setselfref may destroy its unprotected argument (newdt <arg 1>), which is later used. /rchk/packages/build/OFS5pfVE/data.table/src/assign.c:198
  [UP] calling allocating function setselfref with a fresh pointer (newdt <arg 1>) /rchk/packages/build/OFS5pfVE/data.table/src/assign.c:198

Function shift
  [UP] protect stack is too deep, unprotecting all variables, results will be incomplete
  [UP] protect stack is too deep, unprotecting all variables, results will be incomplete
  [UP] unsupported form of unprotect, unprotecting all variables, results will be incomplete /rchk/packages/build/OFS5pfVE/data.table/src/shift.c:174
Analyzed 302 functions, traversed 4262978 states.
Library name (usually package name): data_table
Initialization function: R_init_data_table
Functions: 98
Functions: 1
Checked call to R_registerRoutines: 1

Rchk version: 1cae90e208e97a5c41f1c3e128d99b197478443e
R version: 84255/R Under development (unstable) (2023-04-13 r84255)
LLVM version: 14.0.0

@MichaelChirico MichaelChirico mentioned this pull request Jul 14, 2024
Open
@MichaelChirico
Copy link
Member Author

Seems okay, but why don't we have to protect the copyArg then?

Ah, the wisdom is in CRAN_Release as always:

data.table/.dev/CRAN_Release.cmd

Lines 167 to 173 in 9e2601e

# ScalarInteger and ScalarString allocate and must be PROTECTed unless i) returned (which protects),
# or ii) passed to setAttrib (which protects, providing leak-seals above are ok)
# ScalarLogical in R now returns R's global TRUE from R 3.1.0; Apr 2014. Before that it allocated.
# Aside: ScalarInteger may return globals for small integers in future version of R.
grep ScalarInteger *.c | grep -v PROTECT | grep -v setAttrib | grep -v return # Check all Scalar* either PROTECTed, return-ed or passed to setAttrib.
grep ScalarString *.c | grep -v PROTECT | grep -v setAttrib | grep -v return
grep ScalarLogical *.c # Now we depend on 3.1.0+, check ScalarLogical is NOT PROTECTed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ben-schwen ben-schwen ben-schwen approved these changes

@jangorecki jangorecki Awaiting requested review from jangorecki jangorecki is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@MichaelChirico @ben-schwen