From 4a0ffdd0eb5bdbf12913f7b66ce65b802b59b7a1 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Sat, 9 Nov 2024 09:07:19 -0500 Subject: [PATCH 1/7] Refactor covr_reassign_function to manually swap closure fields for compatibility with non-API functions --- src/reassign.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/src/reassign.c b/src/reassign.c index b4a57549..c67bbf68 100644 --- a/src/reassign.c +++ b/src/reassign.c @@ -1,5 +1,6 @@ #define USE_RINTERNALS #include +#include #include #include #include @@ -9,11 +10,30 @@ SEXP covr_reassign_function(SEXP old_fun, SEXP new_fun) { if (TYPEOF(old_fun) != CLOSXP) error("old_fun must be a function"); if (TYPEOF(new_fun) != CLOSXP) error("new_fun must be a function"); - SET_FORMALS(old_fun, FORMALS(new_fun)); - SET_BODY(old_fun, BODY(new_fun)); - SET_CLOENV(old_fun, CLOENV(new_fun)); + // The goal is to modify `old_fun` in place, so that all existing references + // to `old_fun` call the tracing `new_fun` instead. + // This used to be simply: + // SET_FORMALS(old_fun, FORMALS(new_fun)); + // SET_BODY(old_fun, BODY(new_fun)); + // SET_CLOENV(old_fun, CLOENV(new_fun)); + // But those functions are now "non-API". So we comply with the letter of the law and + // swap the fields manually, making some hard assumptions about the underling memory + // layout in the process. See also: "The Cobra Effect" (https://en.wikipedia.org/wiki/Cobra_effect). + + // Offset and size for closure-specific data within SEXPREC + const size_t closure_data_offset = 32; // 8 bytes (sxpinfo) + 24 bytes (3 pointers for attrib, gengc_next_node, gengc_prev_node) + const size_t closure_data_size = 24; // 3 pointers for formals, body, env (3 * 8 bytes) + + // Duplicate attributes is still not "non-API", thankfully. DUPLICATE_ATTRIB(old_fun, new_fun); + // Temporary buffer to hold CLOSXP data (the 3 pointers to formals, body, env) + char temp[closure_data_size]; + + memcpy(temp, (char *)old_fun + closure_data_offset, closure_data_size); + memcpy((char *)old_fun + closure_data_offset, (char *)new_fun + closure_data_offset, closure_data_size); + memcpy((char *)new_fun + closure_data_offset, temp, closure_data_size); + return R_NilValue; } From 3baaf9a9fecd8a81a8f4803d15f16edd4222dc15 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Sat, 9 Nov 2024 09:08:19 -0500 Subject: [PATCH 2/7] Add workflow_dispatch trigger to R-CMD-check GitHub Actions --- .github/workflows/R-CMD-check.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index ee65ccb5..5df3051d 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.github/workflows/R-CMD-check.yaml @@ -5,6 +5,7 @@ # check-standard.yaml is likely a better choice. # usethis::use_github_action("check-standard") will install it. on: + workflow_dispatch: push: branches: [main, master] pull_request: From 0ad9df62ba87be6057e26efd0a1f645861ec3e2d Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Mon, 11 Nov 2024 10:52:05 -0500 Subject: [PATCH 3/7] mirror struct defs; maybe fix windows issue + other allignment discrepancies --- src/reassign.c | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/src/reassign.c b/src/reassign.c index c67bbf68..c0b74fc6 100644 --- a/src/reassign.c +++ b/src/reassign.c @@ -5,6 +5,30 @@ #include #include #include // for NULL +#include // for uint64_t + +// Mirror the exact structures of SEXPREC from R internals +struct proxy_sxpinfo_struct { + uint64_t bits; // guaranteed to be 64 bits +}; + +struct proxy_closxp_struct { + struct SEXPREC *formals; + struct SEXPREC *body; + struct SEXPREC *env; +}; + +struct proxy_sexprec { + struct proxy_sxpinfo_struct sxpinfo; + struct SEXPREC *attrib; + struct SEXPREC *gengc_next_node, *gengc_prev_node; + union { + struct proxy_closxp_struct closxp; + // We could add other union members if needed + } u; +}; + +typedef struct proxy_sexprec* proxy_sexp; SEXP covr_reassign_function(SEXP old_fun, SEXP new_fun) { if (TYPEOF(old_fun) != CLOSXP) error("old_fun must be a function"); @@ -19,20 +43,19 @@ SEXP covr_reassign_function(SEXP old_fun, SEXP new_fun) { // But those functions are now "non-API". So we comply with the letter of the law and // swap the fields manually, making some hard assumptions about the underling memory // layout in the process. See also: "The Cobra Effect" (https://en.wikipedia.org/wiki/Cobra_effect). - - // Offset and size for closure-specific data within SEXPREC - const size_t closure_data_offset = 32; // 8 bytes (sxpinfo) + 24 bytes (3 pointers for attrib, gengc_next_node, gengc_prev_node) - const size_t closure_data_size = 24; // 3 pointers for formals, body, env (3 * 8 bytes) + // Rather than using memcpy() with a hard coded byte offset, + // we mirror the R internals SEXPREC struct defs here, to hopefully match the alignment + // behavior of R (e.g., on windows). // Duplicate attributes is still not "non-API", thankfully. DUPLICATE_ATTRIB(old_fun, new_fun); - // Temporary buffer to hold CLOSXP data (the 3 pointers to formals, body, env) - char temp[closure_data_size]; + proxy_sexp old = (proxy_sexp) old_fun; + proxy_sexp new = (proxy_sexp) new_fun; - memcpy(temp, (char *)old_fun + closure_data_offset, closure_data_size); - memcpy((char *)old_fun + closure_data_offset, (char *)new_fun + closure_data_offset, closure_data_size); - memcpy((char *)new_fun + closure_data_offset, temp, closure_data_size); + struct proxy_closxp_struct tmp = old->u.closxp; + old->u.closxp = new->u.closxp; + new->u.closxp = tmp; return R_NilValue; } From 9a8e0bc2cf55bcf075ff276e968f617d0df3acd9 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 21 Nov 2024 10:43:53 -0500 Subject: [PATCH 4/7] fix typos in comments Co-authored-by: Marco Colombo --- src/reassign.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/reassign.c b/src/reassign.c index c0b74fc6..59e5ef10 100644 --- a/src/reassign.c +++ b/src/reassign.c @@ -41,9 +41,9 @@ SEXP covr_reassign_function(SEXP old_fun, SEXP new_fun) { // SET_BODY(old_fun, BODY(new_fun)); // SET_CLOENV(old_fun, CLOENV(new_fun)); // But those functions are now "non-API". So we comply with the letter of the law and - // swap the fields manually, making some hard assumptions about the underling memory + // swap the fields manually, making some hard assumptions about the underlying memory // layout in the process. See also: "The Cobra Effect" (https://en.wikipedia.org/wiki/Cobra_effect). - // Rather than using memcpy() with a hard coded byte offset, + // Rather than using memcpy() with a hard-coded byte offset, // we mirror the R internals SEXPREC struct defs here, to hopefully match the alignment // behavior of R (e.g., on windows). From 2f06bc9b874700b935bd69dc7a88e0f339352f04 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 21 Nov 2024 12:47:46 -0500 Subject: [PATCH 5/7] Add memory layout canaries --- src/reassign.c | 131 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 103 insertions(+), 28 deletions(-) diff --git a/src/reassign.c b/src/reassign.c index 59e5ef10..e3144d4f 100644 --- a/src/reassign.c +++ b/src/reassign.c @@ -7,28 +7,60 @@ #include // for NULL #include // for uint64_t -// Mirror the exact structures of SEXPREC from R internals -struct proxy_sxpinfo_struct { - uint64_t bits; // guaranteed to be 64 bits -}; -struct proxy_closxp_struct { - struct SEXPREC *formals; - struct SEXPREC *body; - struct SEXPREC *env; -}; +inline static +void CheckBody(SEXP x) { + switch (TYPEOF(x)) { + case NILSXP: + case SYMSXP: + case LISTSXP: + // case CLOSXP: + case ENVSXP: + case PROMSXP: + case LANGSXP: + // case SPECIALSXP: + // case BUILTINSXP: + case CHARSXP: + case LGLSXP: + case INTSXP: + case REALSXP: + case CPLXSXP: + case STRSXP: + // case DOTSXP: + // case ANYSXP: + case VECSXP: + case EXPRSXP: + case BCODESXP: + case EXTPTRSXP: + case WEAKREFSXP: + case RAWSXP: + case OBJSXP: + return; -struct proxy_sexprec { - struct proxy_sxpinfo_struct sxpinfo; - struct SEXPREC *attrib; - struct SEXPREC *gengc_next_node, *gengc_prev_node; - union { - struct proxy_closxp_struct closxp; - // We could add other union members if needed - } u; -}; + default: + error("Unexpected closure body type"); + } +} + +inline static +void CheckEnvironment(SEXP x) { + if(TYPEOF(x) != ENVSXP) + error("Unexpected closure env type"); +} -typedef struct proxy_sexprec* proxy_sexp; +inline static +void CheckFormals(SEXP ls) { + // copied from R: + // https://github.com/wch/r-source/blob/tags/R-4-4-2/src/main/eval.c#L3842-L3852 + if (isList(ls)) { + for (; ls != R_NilValue; ls = CDR(ls)) + if (TYPEOF(TAG(ls)) != SYMSXP) + goto err; + return; + } + err: + error("Unexpected closure formals"); +} SEXP covr_reassign_function(SEXP old_fun, SEXP new_fun) { if (TYPEOF(old_fun) != CLOSXP) error("old_fun must be a function"); @@ -40,26 +72,69 @@ SEXP covr_reassign_function(SEXP old_fun, SEXP new_fun) { // SET_FORMALS(old_fun, FORMALS(new_fun)); // SET_BODY(old_fun, BODY(new_fun)); // SET_CLOENV(old_fun, CLOENV(new_fun)); - // But those functions are now "non-API". So we comply with the letter of the law and - // swap the fields manually, making some hard assumptions about the underlying memory - // layout in the process. See also: "The Cobra Effect" (https://en.wikipedia.org/wiki/Cobra_effect). - // Rather than using memcpy() with a hard-coded byte offset, - // we mirror the R internals SEXPREC struct defs here, to hopefully match the alignment + // But those functions are now "non-API". So we comply with the letter of the + // law and swap the fields manually, making some hard assumptions about the + // underlying memory layout in the process. + // Rather than using memcpy() with a hard-coded byte offset, we mirror the R + // internals SEXPREC struct defs here, to hopefully match the alignment // behavior of R (e.g., on windows). - // Duplicate attributes is still not "non-API", thankfully. - DUPLICATE_ATTRIB(old_fun, new_fun); + // Mirror the exact structures of SEXPREC from R internals + struct proxy_sxpinfo_struct { + uint64_t bits; // guaranteed to be 64 bits + }; + + struct proxy_closxp_struct { + struct SEXPREC *formals; + struct SEXPREC *body; + struct SEXPREC *env; + }; + + struct proxy_sexprec { + struct proxy_sxpinfo_struct sxpinfo; + struct SEXPREC *attrib; + struct SEXPREC *gengc_next_node, *gengc_prev_node; + union { + struct proxy_closxp_struct closxp; + // We could add other union members if needed + } u; + }; + + typedef struct proxy_sexprec* proxy_sexp; proxy_sexp old = (proxy_sexp) old_fun; proxy_sexp new = (proxy_sexp) new_fun; - struct proxy_closxp_struct tmp = old->u.closxp; + // Sanity checks. If the closxp struct is not what we expect, then the + // underlying internal memory layout of a CLOSXP has probably changed and we + // need to update this code. + // https://github.com/wch/r-source/blob/tags/R-4-4-2/src/include/Defn.h#L170-L174 + CheckFormals(old->u.closxp.formals); + CheckFormals(new->u.closxp.formals); + CheckBody(old->u.closxp.body); + CheckBody(new->u.closxp.body); + CheckEnvironment(old->u.closxp.env); + CheckEnvironment(new->u.closxp.env); + + MARK_NOT_MUTABLE(old_fun); + MARK_NOT_MUTABLE(old->u.closxp.body); + MARK_NOT_MUTABLE(old->u.closxp.env); + MARK_NOT_MUTABLE(old->u.closxp.formals); + + MARK_NOT_MUTABLE(new_fun); + MARK_NOT_MUTABLE(new->u.closxp.body); + MARK_NOT_MUTABLE(new->u.closxp.env); + MARK_NOT_MUTABLE(new->u.closxp.formals); + old->u.closxp = new->u.closxp; - new->u.closxp = tmp; + + // Duplicate attributes is still not "non-API", thankfully. + DUPLICATE_ATTRIB(old_fun, new_fun); return R_NilValue; } + SEXP covr_duplicate_(SEXP x) { return duplicate(x); } /* .Call calls */ From 0fbccb3768bcc75b3ae66541f652f83b22d19ab1 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 21 Nov 2024 12:50:40 -0500 Subject: [PATCH 6/7] Add NEWS --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index 1a9ed599..5e103cde 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,4 +1,7 @@ # covr (development version) + +* Fix R CMD check NOTE: non-API calls to SET_BODY, SET_CLOENV, SET_FORMALS (@t-kalinowski, #587) + * Fix a bug preventing `package_coverage()` from running tests when `install_path` is set to a relative path (@gergness, #517, #548). * Fixed a performance regression and an error triggered by a change in R From d1022680aa0f71fb935c5d936ee584e31afa1221 Mon Sep 17 00:00:00 2001 From: Tomasz Kalinowski Date: Thu, 21 Nov 2024 12:57:38 -0500 Subject: [PATCH 7/7] use `S4SXP` instead of `OBJSXP` for R-oldrel compat. --- src/reassign.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/reassign.c b/src/reassign.c index e3144d4f..21e26065 100644 --- a/src/reassign.c +++ b/src/reassign.c @@ -34,7 +34,7 @@ void CheckBody(SEXP x) { case EXTPTRSXP: case WEAKREFSXP: case RAWSXP: - case OBJSXP: + case S4SXP: // renamed to OBJSXP return; default: @@ -56,7 +56,7 @@ void CheckFormals(SEXP ls) { for (; ls != R_NilValue; ls = CDR(ls)) if (TYPEOF(TAG(ls)) != SYMSXP) goto err; - return; + return; } err: error("Unexpected closure formals");