Close out #33: reduce_one() sanitizer crash, UBSan function-type reports, R-hub workflow

.github/rhub/rxode2-parallel-cse.R

@@ -0,0 +1,39 @@
+## 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")
+## These are rxode2 internals; reach them through the namespace (not `:::`).
+.rx <- asNamespace("rxode2")
+.env <- rxS(.m)
+.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 <- .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")
+}
+cat("rxode2 parallel CSE: OK\n")

.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

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:

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;

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);
 }
 

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;