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

More potential rchk issues #6257

Closed
MichaelChirico opened this issue Jul 14, 2024 · 5 comments
Closed

More potential rchk issues #6257

MichaelChirico opened this issue Jul 14, 2024 · 5 comments
Labels
internals
Milestone
1.16.0

Comments

@MichaelChirico
Copy link
Member 

          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

Originally posted by @ben-schwen in #6249 (comment)
@MichaelChirico MichaelChirico added this to the 1.16.0 milestone Jul 14, 2024
@MichaelChirico
Copy link
Member Author

I'm not sure if these are new in dev, or due to a new version of rchk, or just that rchk only reports certain issues on CRAN. Anyway, we should at least investigate these before release.

@MichaelChirico MichaelChirico mentioned this issue Jul 14, 2024
Merged
MichaelChirico referenced this issue Jul 14, 2024
@MichaelChirico
Remove {UN,}SET_S4_OBJECT usage (#6183)
cd89ceb 
* Remove {UN,}SET_S4_OBJECT usage

* handle asS4 from R side

* tweak NES

* switch to C API

* restore dogroups code using public API

* new test

* adjust NEWS

* IS_S4_OBJECT --> isS4
@MichaelChirico
Copy link
Member Author

@tdhock raises a good point:

should we be running rchk on CI?

I do see {arrow} doing so:

https://github.com/apache/arrow/blob/03726178494c8978bf48b9bab15ed9676e7c9196/dev/tasks/r/github.linux.rchk.yml#L48

They are erroring on 3 different types of rchk outputs ("Suspicious call"/[UP]/[PB])

@jonkeane (original author of that workflow) how has your experience been thus far? Is it reliable & fast (e.g. <10min) enough? Are there false positives?

@jonkeane
Copy link

@jonkeane (original author of that workflow) how has your experience been thus far?

We added this when we were given a 2 week warning shortly after it was introduced. It helped immensely confirming that we passed this check. I don't think we've ever run into a problem with it again (in other words, we haven't introduced anything that caused it to have a true positive since we added it + fixed the issues that were there)

Is it reliable & fast (e.g. <10min) enough?

We run it in our extended checks (called crossbow) which run once a day on main (that is, they aren't part of checks on every commit). Here's an example run, the vast majority of time there is spent installing dependencies (35 minutes of 38 minutes total).

I don't think this job has ever failed outside of something catastrophic like arrow failing to build so it can't even be checked.

Are there false positives?

I've never seen a false positive, and checking for the last 120 days, it has succeeded each day. If you care to, you can confirm this using the crossbow report and searching for rchk in either the build status or logs section to see old runs.

They are erroring on 3 different types of rchk outputs ("Suspicious call"/[UP]/[PB])

We have not gone back to confirm that these are still the only ones that CRAN is checking (or if they've updated the running scripts to allow for a non-clean exit on error. But since we haven't had any issues either with rchks not catching or it erroring otherwise, not invested time to make sure that's the totality of what CRAN cares about for these.

Overall, it's been very lightweight for us and haven't had any false positive or other issues running it.

This was referenced Jul 15, 2024
Merged
Merged
Merged
@MichaelChirico
Copy link
Member Author

Thanks so much @jonkeane! Our build step is pretty fast so this whole GHA only takes ~3 minutes, we'll run it on every push.

@MichaelChirico
Copy link
Member Author

Closing, let's open new issues to improve on memory management further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
internals
Projects
None yet
Milestone
1.16.0
Development

No branches or pull requests
2 participants
@jonkeane @MichaelChirico