assert one tbb runtime rather than none from the stub

CI disproved the premise of the import assertion added in the previous
commit. On both windows-latest configurations -- the Rtools45 oneTBB and the
bundled build -- the downstream library takes its entire TBB surface (13
tbb::detail::r1:: entry points, including isolate_within_arena and observe)
from tbb.dll, and imports nothing at all from RcppParallel.dll. R CMD SHLIB
links RcppParallel.dll against an export list generated from the package's
own objects, so the TBB entry points pulled out of the static library are
never re-exported; '-ltbb' is therefore load-bearing, not a fallback.

So assert the invariant that does hold and does matter: the TBB surface must
come from exactly one module. A library split across RcppParallel.dll and the
stub would register observers with one scheduler while its tasks ran in the
other, which is silent, unlike the link error this replaced.

Two further defects in the check itself:

- an objdump that cannot read the file exited non-zero having printed no
  import table, which parsed as 'no imports' and passed vacuously. This is
  what made the windows-11-arm job green: Sys.which() finds the runner's
  x86_64 objdump, which cannot read an aarch64 PE.
- the export table follows the import tables and lists the library's own
  inlined tbb::detail::d1:: instantiations. Left in, it was absorbed into the
  last 'DLL Name:' block and credited that library with TBB imports it never
  had -- harmless only because tbb.dll happened to be listed last.

Also correct the tbbLdFlags() comment, which described RcppParallel.dll as
re-exporting an incidental part of the runtime.

Author: kevinushey

.github/scripts/tbb-downstream-check.R

@@ -97,37 +97,72 @@ if (status != 0L)
 
 dllName <- paste0("check", .Platform$dynlib.ext)
 
-# Check where the TBB symbols actually came from. RcppParallelLibs() offers
-# the 'tbb.dll' stub after '-lRcppParallel', so anything RcppParallel.dll
-# failed to re-export still links -- but the stub carries its own copy of the
-# oneTBB runtime, so binding to it means running against a second, independent
-# scheduler. That is a silent failure, unlike the link error it replaced, so
-# assert here that the whole TBB surface resolved from RcppParallel.dll.
+# Report which module supplied the TBB symbols, and fail if that is more than
+# one. On Windows the surface can legitimately come from either RcppParallel.dll
+# (which links TBB statically) or the 'tbb.dll' stub that RcppParallelLibs()
+# offers after '-lRcppParallel' -- in practice it is the stub, since
+# RcppParallel.dll turns out not to re-export the runtime at all, which is why
+# '-ltbb' is load-bearing rather than a fallback. What must not happen is a
+# library split across both: they are separate copies of the oneTBB runtime, so
+# an observer registered with one would never fire for arenas owned by the
+# other, and unlike the link error this replaced, that failure is silent.
 if (.Platform$OS.type == "windows") {
 
    objdump <- Sys.which("objdump")
+   output <- if (nzchar(objdump))
+      suppressWarnings(system2(objdump, c("-p", shQuote(dllName)), stdout = TRUE, stderr = TRUE))
+
+   status <- attr(output, "status")
+
+   # keep only the import tables. the export table follows them, and lists this
+   # library's own inlined TBB instantiations -- left in, it would be absorbed
+   # into the last 'DLL Name:' block and credit that library with TBB symbols
+   # it never imported
+   exports <- grep("export table|The Export Tables", output)
+   if (length(exports))
+      output <- output[seq_len(exports[[1L]] - 1L)]
+
+   # each imported library opens a 'DLL Name:' block listing the symbols taken
+   # from it, and runs until the next such block
+   starts <- grep("DLL Name:", output, fixed = TRUE)
+   readable <- length(starts) > 0L && !(is.numeric(status) && status != 0L)
+
    if (!nzchar(objdump)) {
 
       writeLines("** objdump not found; skipping the import table check")
 
-   } else {
+   } else if (!readable) {
 
-      output <- system2(objdump, c("-p", shQuote(dllName)), stdout = TRUE, stderr = TRUE)
+      # don't let an objdump that couldn't read the file pass as 'no imports':
+      # the runner's objdump may be built for another target (an x86_64 one
+      # cannot read an aarch64 PE, and exits non-zero having printed nothing)
+      fmt <- "** '%s' could not read the import table of '%s'; skipping the check"
+      writeLines(sprintf(fmt, objdump, dllName))
+      writeLines(output)
 
-      # each imported library opens a 'DLL Name:' block listing the symbols
-      # taken from it, and runs until the next such block
-      starts <- grep("DLL Name:", output, fixed = TRUE)
-      imported <- sub(".*DLL Name:[[:space:]]*", "", output[starts])
-      writeLines(sprintf("imports: %s", paste(imported, collapse = ", ")))
+   } else {
 
-      index <- which(tolower(imported) == "tbb.dll")
-      if (length(index)) {
-         ends <- c(starts[-1L], length(output) + 1L)
-         writeLines(output[seq(starts[[index]], ends[[index]] - 1L)])
-         stop("the downstream library imports the above from the tbb.dll stub; ",
-              "these should have resolved from RcppParallel.dll instead")
+      imported <- sub(".*DLL Name:[[:space:]]*", "", output[starts])
+      ends <- c(starts[-1L], length(output) + 1L)
+
+      # attribute the TBB symbols to the module each was taken from
+      providers <- character()
+      for (i in seq_along(starts)) {
+         block <- output[seq(starts[[i]], ends[[i]] - 1L)]
+         if (any(grepl("_ZN3tbb", block, fixed = TRUE)))
+            providers <- c(providers, imported[[i]])
       }
 
+      writeLines(sprintf("imports: %s", paste(imported, collapse = ", ")))
+      writeLines(sprintf("tbb symbols from: %s", if (length(providers))
+         paste(providers, collapse = ", ") else "(none; resolved statically)"))
+
+      if (length(providers) > 1L)
+         stop("the downstream library takes TBB symbols from more than one ",
+              "module (", paste(providers, collapse = ", "), "); those are ",
+              "separate copies of the oneTBB runtime, so its observers and its ",
+              "arenas would belong to different schedulers")
+
    }
 
 }

R/tbb.R

@@ -92,21 +92,20 @@ tbbLdFlags <- function() {
 
       ldFlags <- sprintf("-L%s -lRcppParallel", asBuildPath(libPath))
 
-      # also offer the stub library. RcppParallel.dll re-exports whichever TBB
-      # objects happened to be pulled out of the static library when it was
-      # linked, which is an incidental export surface rather than a declared
-      # one; the stub exports the runtime wholesale, so anything missing from
-      # RcppParallel.dll can still be resolved. this comes last deliberately:
-      # the linker satisfies symbols in order, so RcppParallel's own exports
-      # still win, and no import of the stub is recorded unless something
-      # actually needs it.
+      # also offer the stub library, which exports the TBB runtime wholesale.
+      # this is not just a safety net: R CMD SHLIB links RcppParallel.dll
+      # against an export list generated from our own objects, so the TBB
+      # entry points pulled in from the static library are not re-exported,
+      # and a downstream package linking '-lRcppParallel' alone resolves none
+      # of them (which is what broke rstan). the stub is what makes those
+      # symbols reachable at all.
       #
-      # that ordering matters for more than tidiness. the stub links its own
-      # copy of the oneTBB runtime, so a package resolving some symbols here
-      # and the rest against RcppParallel.dll would straddle two independent
-      # schedulers -- an observer registered with one would never fire for
-      # arenas owned by the other. .github/scripts/tbb-downstream-check.R
-      # asserts that nothing actually lands on the stub
+      # it still comes last, so that anything RcppParallel.dll does export
+      # wins, and so no import of the stub is recorded unless something needs
+      # it. that matters because the stub carries its own copy of the oneTBB
+      # runtime: a package split across both would register observers with one
+      # scheduler while its tasks ran in the other.
+      # .github/scripts/tbb-downstream-check.R asserts that does not happen
       tbbPath <- archSystemFile("lib")
       if (file.exists(file.path(tbbPath, "tbb.dll")))
          ldFlags <- paste(ldFlags, sprintf("-L%s -ltbb", asBuildPath(tbbPath)))