From e600b946c47973e98f34447aa178063811e59fc7 Mon Sep 17 00:00:00 2001 From: mattfidler Date: Mon, 5 Oct 2026 08:59:34 -0500 Subject: [PATCH 1/2] Close out #33: sequence build_paths, exact hash callback types, R-hub workflow The reduce_one() crash R-hub's clang-asan/clang-ubsan checks hit in babelmixr2 (#33) is the shared first-path race fixed in #32; reproduced on 1.3.1-13 and clean on 1.3.1-14 under clang ASan/UBSan, both with the concurrent test and with rxode2's parallel CSE. - build_paths(): call new_VecZNode() before vec_add(); the n++ and the call were unsequenced, so gcc never used path1 and the concurrent test could only catch the race on clang. - lex.c/write_tables.c: hash/cmp callbacks take void* to match hash_fn_t/cmp_fn_t, silencing clang -fsanitize=function in mkdparse(). - .github/workflows/rhub.yaml: R-hub workflow with NOT_CRAN=true and a downstream rxode2 parallel CSE smoke test (.github/rhub/). --- .github/rhub/rxode2-parallel-cse.R | 35 ++++++++ .github/workflows/rhub.yaml | 137 +++++++++++++++++++++++++++++ NEWS.md | 21 ++++- src/lex.c | 9 +- src/parse.c | 9 +- src/write_tables.c | 45 ++++++---- 6 files changed, 235 insertions(+), 21 deletions(-) create mode 100644 .github/rhub/rxode2-parallel-cse.R create mode 100644 .github/workflows/rhub.yaml diff --git a/.github/rhub/rxode2-parallel-cse.R b/.github/rhub/rxode2-parallel-cse.R new file mode 100644 index 0000000..ca15ae5 --- /dev/null +++ b/.github/rhub/rxode2-parallel-cse.R @@ -0,0 +1,35 @@ +## Downstream smoke test for nlmixr2/dparser-R#33. +## +## rxode2's C common-subexpression pass (rxCse) calls dparse() once per +## statement from inside an OpenMP region. With dparser 1.3.1-13 that crashed +## in reduce_one() on R-hub's clang-asan/clang-ubsan containers (seen first in +## babelmixr2's checks). .github/workflows/rhub.yaml runs this after R CMD +## check, with this checkout's dparser and rxode2 from GitHub installed, so the +## crash shows up here instead of downstream. Modeled on rxode2's +## "the C pass is independent of the thread count" test. +library(rxode2) +cat("dparser", format(packageVersion("dparser")), + "rxode2", format(packageVersion("rxode2")), "\n") + +.m <- paste(c( + "a <- exp(p1 + p2)", + "b <- exp(p1 + p2) + exp(p3 + p4)", + "d/dt(x) <- -exp(p1 + p2)*x + exp(p3 + p4)*a", + "d/dt(y) <- exp(p3 + p4)*x - exp(p1 + p2)*y", + "cp <- x/a + y/b" +), collapse = "\n") +.v <- c("p1", "p2", "p3") +.env <- rxS(.m) +.txt <- paste(c(.m, rxode2:::.rxJacobian(.env), rxode2:::.rxSens(.env, .v), + rxode2:::.rxSens(.env, .v, .v)), collapse = "\n") +.norm <- rxNorm(.txt) +cat("statements:", length(strsplit(.norm, "\n", fixed = TRUE)[[1]]), "\n") + +.res <- vapply(rep(c(1L, 2L, 4L, 8L), 5L), function(n) { + setRxThreads(n) + .o <- rxode2:::.rxOptExprC(.norm) + if (is.na(.o)) NA_character_ else .o +}, character(1)) +if (anyNA(.res)) stop("rxode2's C CSE pass declined the model") +if (length(unique(.res)) != 1L) stop("rxode2's C CSE pass depends on the thread count") +cat("rxode2 parallel CSE: OK\n") diff --git a/.github/workflows/rhub.yaml b/.github/workflows/rhub.yaml new file mode 100644 index 0000000..bda05d6 --- /dev/null +++ b/.github/workflows/rhub.yaml @@ -0,0 +1,137 @@ +# R-hub's generic GitHub Actions workflow file. It's canonical location is at +# https://github.com/r-hub/actions/blob/v1/workflows/rhub.yaml +# You can update this file to a newer version using the rhub2 package: +# +# rhub::rhub_setup() +# +# dparser changes (keep them when updating): +# +# * NOT_CRAN=true, so tests/testthat/test-concurrent-dparse.R (skip_on_cran) +# runs; it is what catches concurrent dparse() bugs (nlmixr2/dparser-R#33). +# +# * After the check, a downstream step installs this checkout's dparser and +# rxode2 from GitHub and runs .github/rhub/rxode2-parallel-cse.R, rxode2's +# OpenMP common-subexpression pass, which is where #33 crashed first (in +# babelmixr2's checks). Turn it off with the `downstream` input. +# +# For the sanitizer checks, e.g. +# +# rhub::rhub_check(platforms = c("clang-asan", "clang-ubsan")) + +name: R-hub +run-name: "${{ github.event.inputs.id }}: ${{ github.event.inputs.name || format('Manually run by {0}', github.triggering_actor) }}" + +on: + workflow_dispatch: + inputs: + config: + description: 'A comma separated list of R-hub platforms to use.' + type: string + default: 'linux,windows,macos' + name: + description: 'Run name. You can leave this empty now.' + type: string + id: + description: 'Unique ID. You can leave this empty now.' + type: string + downstream: + description: 'Also run the rxode2 parallel CSE smoke test (not on Windows).' + type: string + default: 'true' + +jobs: + + setup: + runs-on: ubuntu-latest + outputs: + containers: ${{ steps.rhub-setup.outputs.containers }} + platforms: ${{ steps.rhub-setup.outputs.platforms }} + + steps: + # NO NEED TO CHECKOUT HERE + - uses: r-hub/actions/setup@v1 + with: + config: ${{ github.event.inputs.config }} + id: rhub-setup + + linux-containers: + needs: setup + if: ${{ needs.setup.outputs.containers != '[]' }} + runs-on: ubuntu-latest + name: ${{ matrix.config.label }} + strategy: + fail-fast: false + matrix: + config: ${{ fromJson(needs.setup.outputs.containers) }} + container: + image: ${{ matrix.config.container }} + env: + NOT_CRAN: true + + steps: + - uses: r-hub/actions/checkout@v1 + - uses: r-hub/actions/platform-info@v1 + with: + token: ${{ secrets.RHUB_TOKEN }} + job-config: ${{ matrix.config.job-config }} + - uses: r-hub/actions/setup-deps@v1 + with: + token: ${{ secrets.RHUB_TOKEN }} + job-config: ${{ matrix.config.job-config }} + - uses: r-hub/actions/run-check@v1 + with: + token: ${{ secrets.RHUB_TOKEN }} + job-config: ${{ matrix.config.job-config }} + - name: Downstream rxode2 parallel CSE (dparser-R#33) + if: ${{ !cancelled() && github.event.inputs.downstream != 'false' }} + shell: bash + run: | + set -o pipefail + Rscript -e 'pak::pak(c("local::.", "nlmixr2/rxode2"), upgrade = FALSE, ask = FALSE)' + Rscript .github/rhub/rxode2-parallel-cse.R 2>&1 | tee downstream.out + if grep -E 'runtime error|ERROR: AddressSanitizer' downstream.out; then + echo "::error::sanitizer report in the rxode2 parallel CSE smoke test" + exit 1 + fi + + other-platforms: + needs: setup + if: ${{ needs.setup.outputs.platforms != '[]' }} + runs-on: ${{ matrix.config.os }} + name: ${{ matrix.config.label }} + strategy: + fail-fast: false + matrix: + config: ${{ fromJson(needs.setup.outputs.platforms) }} + env: + NOT_CRAN: true + + steps: + - uses: r-hub/actions/checkout@v1 + - uses: r-hub/actions/setup-r@v1 + with: + job-config: ${{ matrix.config.job-config }} + token: ${{ secrets.RHUB_TOKEN }} + - uses: r-hub/actions/platform-info@v1 + with: + token: ${{ secrets.RHUB_TOKEN }} + job-config: ${{ matrix.config.job-config }} + - uses: r-hub/actions/setup-deps@v1 + with: + job-config: ${{ matrix.config.job-config }} + token: ${{ secrets.RHUB_TOKEN }} + - uses: r-hub/actions/run-check@v1 + with: + job-config: ${{ matrix.config.job-config }} + token: ${{ secrets.RHUB_TOKEN }} + - name: Downstream rxode2 parallel CSE (dparser-R#33) + if: ${{ !cancelled() && runner.os != 'Windows' && github.event.inputs.downstream != 'false' }} + shell: bash + run: | + set -o pipefail + Rscript -e 'pak::pak(c("local::.", "nlmixr2/rxode2"), upgrade = FALSE, ask = FALSE)' + Rscript .github/rhub/rxode2-parallel-cse.R 2>&1 | tee downstream.out + if grep -E 'runtime error|ERROR: AddressSanitizer' downstream.out; then + echo "::error::sanitizer report in the rxode2 parallel CSE smoke test" + exit 1 + fi diff --git a/NEWS.md b/NEWS.md index b83e12c..0adaf7e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -7,7 +7,26 @@ on the reducing call's stack. Callers must resolve the `dparser.h` entry points on the main thread first, and syntax errors, ambiguity errors and R-level callbacks still call the R API, so only error-free parses without - R callbacks are safe off the main thread. + R callbacks are safe off the main thread. This is also the + `reduce_one()` global-buffer-overflow / SEGV that R-hub's `clang-asan` + and `clang-ubsan` checks of babelmixr2 hit (#33). Only clang builds + ever used the shared vector -- `vec_add(paths, new_VecZNode(paths, ...))` + left the order of `n++` and the call to the compiler, and gcc builds took + a fresh `malloc()`ed path instead -- which is why it showed up on clang + platforms only. `build_paths()` now makes the call first, so every + compiler takes the same path and the concurrent test exercises it on gcc + too. + +- The grammar compiler's hash callbacks (`lex.c`, `write_tables.c`) now + have the exact `hash_fn_t` / `cmp_fn_t` types instead of being cast to + them, so clang's `-fsanitize=function` (part of `-fsanitize=undefined`) + no longer reports "call to function through pointer to incorrect + function type" from `mkdparse()`. + +- A manually dispatched R-hub workflow (`rhub::rhub_check()`) runs the + concurrent `dparse()` test (`NOT_CRAN=true`) and then rxode2's OpenMP + common-subexpression pass against this dparser, so sanitizer problems + are caught here before they reach rxode2/babelmixr2 checks (#33). - `buf_read()` (and therefore `sbuf_read()`) is hardened in three ways without changing its `int *len` ABI: diff --git a/src/lex.c b/src/lex.c index b6614b0..9d0808d 100644 --- a/src/lex.c +++ b/src/lex.c @@ -341,7 +341,8 @@ static void compute_liveness(Scanner *scanner) { } } -static uint32 trans_hash_fn(ScanStateTransition *a, hash_fns_t *fns) { +static uint32 trans_hash_fn(void *va, hash_fns_t *fns) { + ScanStateTransition *a = (ScanStateTransition *)va; uint h = 0, i; if (!fns->data[0]) @@ -350,7 +351,9 @@ static uint32 trans_hash_fn(ScanStateTransition *a, hash_fns_t *fns) { return h; } -static int trans_cmp_fn(ScanStateTransition *a, ScanStateTransition *b, hash_fns_t *fns) { +static int trans_cmp_fn(void *va, void *vb, hash_fns_t *fns) { + ScanStateTransition *a = (ScanStateTransition *)va; + ScanStateTransition *b = (ScanStateTransition *)vb; uint i; if (!fns->data[0]) @@ -364,7 +367,7 @@ static int trans_cmp_fn(ScanStateTransition *a, ScanStateTransition *b, hash_fns return 0; } -static hash_fns_t trans_hash_fns = {(hash_fn_t)trans_hash_fn, (cmp_fn_t)trans_cmp_fn, {0, 0}}; +static hash_fns_t trans_hash_fns = {trans_hash_fn, trans_cmp_fn, {0, 0}}; static void build_transitions(LexState *ls, Scanner *s) { uint i, j; diff --git a/src/parse.c b/src/parse.c index c637a0b..7b06df4 100644 --- a/src/parse.c +++ b/src/parse.c @@ -1344,7 +1344,8 @@ static void build_paths_internal(ZNode *z, VecVecZNode *paths, int parent, int n for (j = 0, l = 0; j < z->sns.v[k]->zns.n; j++) { if (z->sns.v[k]->zns.v[j]) { if (k + l) { - vec_add(paths, new_VecZNode(paths, n - (n_to_go - 1), parent, path1)); + VecZNode *pv = new_VecZNode(paths, n - (n_to_go - 1), parent, path1); + vec_add(paths, pv); parent = paths->n - 1; } build_paths_internal(z->sns.v[k]->zns.v[j], paths, parent, n, n_to_go - 1, path1); @@ -1354,8 +1355,12 @@ static void build_paths_internal(ZNode *z, VecVecZNode *paths, int parent, int n } static void build_paths(ZNode *z, VecVecZNode *paths, int nchildren_to_go, VecZNode *path1) { + VecZNode *pv; if (!nchildren_to_go) return; - vec_add(paths, new_VecZNode(paths, 0, -1, path1)); + /* not vec_add(paths, new_VecZNode(...)): vec_add's n++ and the call are + unsequenced, so whether path1 was used depended on the compiler */ + pv = new_VecZNode(paths, 0, -1, path1); + vec_add(paths, pv); build_paths_internal(z, paths, 0, nchildren_to_go, nchildren_to_go, path1); } diff --git a/src/write_tables.c b/src/write_tables.c index 8c57587..64b078a 100644 --- a/src/write_tables.c +++ b/src/write_tables.c @@ -54,17 +54,20 @@ OffsetEntry null_entry = {"NULL", sizeof("NULL") - 1, -1}; OffsetEntry spec_code_entry = {"#spec_code", sizeof("#spec_code") - 1, -2}; OffsetEntry final_code_entry = {"#final_code", sizeof("#final_code") - 1, -3}; -uint32 offset_hash_fn(OffsetEntry *entry, struct hash_fns_t *fn) { +uint32 offset_hash_fn(void *ventry, struct hash_fns_t *fn) { + OffsetEntry *entry = (OffsetEntry *)ventry; (void)fn; return strhashl(entry->name, entry->len); } -int offset_cmp_fn(OffsetEntry *a, OffsetEntry *b, struct hash_fns_t *fn) { +int offset_cmp_fn(void *va, void *vb, struct hash_fns_t *fn) { + OffsetEntry *a = (OffsetEntry *)va; + OffsetEntry *b = (OffsetEntry *)vb; (void)fn; return strcmp(a->name, b->name); } -hash_fns_t offset_fns = {(hash_fn_t)offset_hash_fn, (cmp_fn_t)offset_cmp_fn, {0, 0}}; +hash_fns_t offset_fns = {offset_hash_fn, offset_cmp_fn, {0, 0}}; static void write_chk(const void *ptr, size_t size, size_t nmemb, File *file) { if (file->fp) { @@ -492,7 +495,8 @@ static char *make_u_type(int i) { static char *scanner_u_type(State *s) { return make_u_type(scanner_size(s)); } -static uint32 scanner_block_hash_fn(ScannerBlock *b, hash_fns_t *fns) { +static uint32 scanner_block_hash_fn(void *vb, hash_fns_t *fns) { + ScannerBlock *b = (ScannerBlock *)vb; uint32 hash = 0; intptr_t i, block_size = (intptr_t)fns->data[0]; ScanState **sb = b->chars; @@ -504,7 +508,9 @@ static uint32 scanner_block_hash_fn(ScannerBlock *b, hash_fns_t *fns) { return hash; } -static int scanner_block_cmp_fn(ScannerBlock *a, ScannerBlock *b, hash_fns_t *fns) { +static int scanner_block_cmp_fn(void *va, void *vb, hash_fns_t *fns) { + ScannerBlock *a = (ScannerBlock *)va; + ScannerBlock *b = (ScannerBlock *)vb; intptr_t i, block_size = (intptr_t)fns->data[0]; ScanState **sa = a->chars; ScanState **sb = b->chars; @@ -517,9 +523,10 @@ static int scanner_block_cmp_fn(ScannerBlock *a, ScannerBlock *b, hash_fns_t *fn return 0; } -hash_fns_t scanner_block_fns = {(hash_fn_t)scanner_block_hash_fn, (cmp_fn_t)scanner_block_cmp_fn, {0, 0}}; +hash_fns_t scanner_block_fns = {scanner_block_hash_fn, scanner_block_cmp_fn, {0, 0}}; -static uint32 trans_scanner_block_hash_fn(ScannerBlock *b, hash_fns_t *fns) { +static uint32 trans_scanner_block_hash_fn(void *vb, hash_fns_t *fns) { + ScannerBlock *b = (ScannerBlock *)vb; uint32 hash = 0; intptr_t i, block_size = (intptr_t)fns->data[0]; ScanStateTransition **sb = b->transitions; @@ -531,7 +538,9 @@ static uint32 trans_scanner_block_hash_fn(ScannerBlock *b, hash_fns_t *fns) { return hash; } -static int trans_scanner_block_cmp_fn(ScannerBlock *a, ScannerBlock *b, hash_fns_t *fns) { +static int trans_scanner_block_cmp_fn(void *va, void *vb, hash_fns_t *fns) { + ScannerBlock *a = (ScannerBlock *)va; + ScannerBlock *b = (ScannerBlock *)vb; intptr_t i, block_size = (intptr_t)fns->data[0]; ScanStateTransition **sa = a->transitions; ScanStateTransition **sb = b->transitions; @@ -545,19 +554,22 @@ static int trans_scanner_block_cmp_fn(ScannerBlock *a, ScannerBlock *b, hash_fns } hash_fns_t trans_scanner_block_fns = { - (hash_fn_t)trans_scanner_block_hash_fn, (cmp_fn_t)trans_scanner_block_cmp_fn, {0, 0}}; + trans_scanner_block_hash_fn, trans_scanner_block_cmp_fn, {0, 0}}; -static uint32 shift_hash_fn(Action *sa, hash_fns_t *fns) { +static uint32 shift_hash_fn(void *vsa, hash_fns_t *fns) { + Action *sa = (Action *)vsa; (void)fns; return sa->term->index + (sa->kind == ACTION_SHIFT_TRAILING ? 1000000 : 0); } -static int shift_cmp_fn(Action *sa, Action *sb, hash_fns_t *fns) { +static int shift_cmp_fn(void *vsa, void *vsb, hash_fns_t *fns) { + Action *sa = (Action *)vsa; + Action *sb = (Action *)vsb; (void)fns; return (sa->term->index != sb->term->index) || (sa->kind != sb->kind); } -hash_fns_t shift_fns = {(hash_fn_t)shift_hash_fn, (cmp_fn_t)shift_cmp_fn, {0, 0}}; +hash_fns_t shift_fns = {shift_hash_fn, shift_cmp_fn, {0, 0}}; static void write_scanner_data(File *fp, Grammar *g, char *tag) { State *s; @@ -1356,7 +1368,8 @@ static void write_reductions(File *file, Grammar *g, char *tag) { } } -static uint32 er_hint_hash_fn(State *a, hash_fns_t *fns) { +static uint32 er_hint_hash_fn(void *va, hash_fns_t *fns) { + State *a = (State *)va; VecHint *sa = &a->error_recovery_hints; uint32 hash = 0, i; Term *ta; @@ -1371,7 +1384,9 @@ static uint32 er_hint_hash_fn(State *a, hash_fns_t *fns) { return hash; } -static int er_hint_cmp_fn(State *a, State *b, hash_fns_t *fns) { +static int er_hint_cmp_fn(void *va, void *vb, hash_fns_t *fns) { + State *a = (State *)va; + State *b = (State *)vb; uint i; VecHint *sa = &a->error_recovery_hints, *sb = &b->error_recovery_hints; Term *ta, *tb; @@ -1387,7 +1402,7 @@ static int er_hint_cmp_fn(State *a, State *b, hash_fns_t *fns) { return 0; } -hash_fns_t er_hint_hash_fns = {(hash_fn_t)er_hint_hash_fn, (cmp_fn_t)er_hint_cmp_fn, {0, 0}}; +hash_fns_t er_hint_hash_fns = {er_hint_hash_fn, er_hint_cmp_fn, {0, 0}}; static void write_error_data(File *fp, Grammar *g, VecState *er_hash, char *tag) { uint i, j; From 867ab9c254e7506957ba57f57e0e539e35cf49e1 Mon Sep 17 00:00:00 2001 From: mattfidler Date: Mon, 5 Oct 2026 12:41:36 -0500 Subject: [PATCH 2/2] rhub smoke test: reach rxode2 internals via asNamespace, not ::: Clears the 4 CodeFactor undesirable-operator issues on #34. --- .github/rhub/rxode2-parallel-cse.R | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/.github/rhub/rxode2-parallel-cse.R b/.github/rhub/rxode2-parallel-cse.R index ca15ae5..eaa3c7a 100644 --- a/.github/rhub/rxode2-parallel-cse.R +++ b/.github/rhub/rxode2-parallel-cse.R @@ -19,17 +19,21 @@ cat("dparser", format(packageVersion("dparser")), "cp <- x/a + y/b" ), collapse = "\n") .v <- c("p1", "p2", "p3") +## These are rxode2 internals; reach them through the namespace (not `:::`). +.rx <- asNamespace("rxode2") .env <- rxS(.m) -.txt <- paste(c(.m, rxode2:::.rxJacobian(.env), rxode2:::.rxSens(.env, .v), - rxode2:::.rxSens(.env, .v, .v)), collapse = "\n") +.txt <- paste(c(.m, .rx$.rxJacobian(.env), .rx$.rxSens(.env, .v), + .rx$.rxSens(.env, .v, .v)), collapse = "\n") .norm <- rxNorm(.txt) cat("statements:", length(strsplit(.norm, "\n", fixed = TRUE)[[1]]), "\n") .res <- vapply(rep(c(1L, 2L, 4L, 8L), 5L), function(n) { setRxThreads(n) - .o <- rxode2:::.rxOptExprC(.norm) + .o <- .rx$.rxOptExprC(.norm) if (is.na(.o)) NA_character_ else .o }, character(1)) if (anyNA(.res)) stop("rxode2's C CSE pass declined the model") -if (length(unique(.res)) != 1L) stop("rxode2's C CSE pass depends on the thread count") +if (length(unique(.res)) != 1L) { + stop("rxode2's C CSE pass depends on the thread count") +} cat("rxode2 parallel CSE: OK\n")