build the bundled onetbb as a shared library on windows

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

@@ -97,77 +97,141 @@ if (status != 0L)
 
 dllName <- paste0("check", .Platform$dynlib.ext)
 
-# 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))
+# On Windows, check where the TBB symbols came from. RcppParallel builds oneTBB
+# as a shared library and links against it, exactly as on other platforms, so
+# there should be a single TBB runtime in the process: this library's TBB
+# symbols must all come from 'tbb.dll', and RcppParallel.dll must import from it
+# too rather than carrying a copy of its own. Two runtimes would be a silent
+# failure -- an observer registered with one would never fire for arenas owned
+# by the other -- so it is worth asserting rather than assuming.
+
+# objdump has to match the target architecture, and the first one on the PATH
+# may well not: the aarch64 runner leads with an x86_64 objdump from C:/mingw64,
+# which exits non-zero having read nothing. Ask R which compiler it builds with
+# -- that is the one that produced the PE, so its objdump can always read it --
+# and only then fall back to whatever the PATH offers
+objdumpCandidates <- function() {
+
+   cc <- suppressWarnings(
+      system2(
+         file.path(R.home("bin"), "R"),
+         c("CMD", "config", "CC"),
+         stdout = TRUE,
+         stderr = FALSE
+      )
+   )
+
+   # CC may carry arguments (e.g. 'gcc -std=gnu2x'), and may name the compiler
+   # rather than spell out its path, in which case the PATH has to resolve it
+   cc <- strsplit(trimws(paste(cc, collapse = " ")), "[[:space:]]+")[[1L]][[1L]]
+   cc <- if (file.exists(cc)) cc else Sys.which(cc)
+
+   compilers <- c(cc, Sys.which(c("gcc", "clang", "cc")))
+   beside <- file.path(dirname(compilers[nzchar(compilers)]), "objdump.exe")
+
+   candidates <- c(beside, Sys.which("objdump"))
+   candidates <- candidates[nzchar(candidates) & file.exists(candidates)]
+
+   # '/' and '\\' spellings of one path are the same objdump; don't try twice
+   unique(normalizePath(candidates, winslash = "/", mustWork = FALSE))
 
-   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)]
+tbbImports <- function(dll) {
 
    # 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)
+   # from it; finding at least one is how we know objdump really read the file,
+   # rather than failing in a way that would look like 'imports nothing'
+   starts <- integer()
+   output <- character()
+
+   for (objdump in objdumpCandidates()) {
+
+      output <- suppressWarnings(
+         system2(objdump, c("-p", shQuote(dll)), stdout = TRUE, stderr = TRUE)
+      )
+      status <- attr(output, "status")
+
+      # keep only the import tables. the export table follows them, and lists
+      # the 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)]
+
+      starts <- grep("DLL Name:", output, fixed = TRUE)
+      if (length(starts) && !(is.numeric(status) && status != 0L))
+         break
+
+      fmt <- "** '%s' could not read the import table of '%s'"
+      writeLines(sprintf(fmt, objdump, basename(dll)))
+      writeLines(output)
 
-   if (!nzchar(objdump)) {
+      starts <- integer()
 
-      writeLines("** objdump not found; skipping the import table check")
+   }
+
+   if (!length(starts)) {
+      fmt <- "** no usable objdump for '%s'; skipping the import table check"
+      writeLines(sprintf(fmt, basename(dll)))
+      return(NULL)
+   }
 
-   } else if (!readable) {
+   imported <- sub(".*DLL Name:[[:space:]]*", "", output[starts])
+   ends <- c(starts[-1L], length(output) + 1L)
 
-      # 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)
+   # 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]])
+   }
+
+   fmt <- "%s imports: %s [tbb symbols from: %s]"
+   writeLines(sprintf(
+      fmt, basename(dll),
+      paste(imported, collapse = ", "),
+      if (length(providers)) paste(providers, collapse = ", ") else "none"
+   ))
+
+   tolower(providers)
+
+}
 
-   } else {
+if (.Platform$OS.type == "windows") {
 
-      imported <- sub(".*DLL Name:[[:space:]]*", "", output[starts])
-      ends <- c(starts[-1L], length(output) + 1L)
+   providers <- tbbImports(dllName)
 
-      # 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]])
-      }
+   if (!is.null(providers)) {
 
-      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("expected this library's tbb symbols to come from exactly one ",
+              "module, but found ", length(providers), " (",
+              paste(providers, collapse = ", "), "); more than one means more ",
+              "than one copy of the oneTBB runtime")
 
-      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")
+      if (!identical(providers, "tbb.dll"))
+         stop("this library's tbb symbols came from '", providers,
+              "' rather than the shared 'tbb.dll' runtime")
 
    }
 
+   # and RcppParallel itself must be a client of that same runtime
+   rcppParallelDll <- RcppParallel:::archSystemFile("libs", "RcppParallel.dll")
+   if (!file.exists(rcppParallelDll))
+      stop("could not locate RcppParallel.dll within the installed package")
+
+   providers <- tbbImports(rcppParallelDll)
+   if (!is.null(providers) && !identical(providers, "tbb.dll"))
+      stop("RcppParallel.dll does not take its tbb symbols from 'tbb.dll' ",
+           "(got: ", paste(providers, collapse = ", "),
+           "); it appears to carry its own copy of the runtime")
+
 }
 
-# loading also proves any load-time dependency on the tbb stub resolves
+# loading also proves the load-time dependency on the tbb runtime resolves
 dll <- dyn.load(dllName)
 on.exit(dyn.unload(dll[["path"]]), add = TRUE)
 

NEWS.md

@@ -8,23 +8,37 @@
   loaded into the process, and failed with "Library not loaded:
   @rpath/libtbb.dylib" otherwise. (#209)
 
-* Fixed `RcppParallel::tbbLibraryPath()` returning `NULL` on Windows. It looked
-  for the static archives `libtbb12.a` / `libtbb.a`, which are never installed
-  with the package: TBB is linked statically into `RcppParallel.dll` there, and
-  the only TBB library installed alongside it is the `tbb.dll` stub, which is
-  what it now finds. Previously it succeeded only when the package happened to
-  be running on the machine that built it against an Rtools TBB, and even then
-  answered with a path into Rtools -- a build dependency -- rather than with
-  anything the installed package uses. Note that `tbbLibraryPath("tbbmalloc")`
-  returns `NULL` on Windows, as no separate tbbmalloc library is installed
-  there. (#270)
-
-* `RcppParallel::tbbLibraryPath()` called with no arguments, and the internal
-  `tbbRoot()`, no longer report the Rtools directory recorded when the package
-  was configured. Unlike the named-library form, these return their answer
-  without checking that it exists, so for a pre-built binary (e.g. the CRAN
-  build) they named a directory on the machine that built the package. On
-  Windows both now report the package's own library directory. (#270)
+* On Windows, RcppParallel now builds the bundled oneTBB as a shared library
+  and links against it, shipping `tbb.dll` and `tbbmalloc.dll` alongside the
+  package -- the same arrangement already used on every other platform.
+  Previously it linked the static TBB provided by Rtools directly into
+  `RcppParallel.dll`, which meant the TBB version (and ABI) depended on the
+  user's toolchain, and left downstream packages with no TBB library to link
+  against. Building it ourselves gives every platform the same oneTBB and makes
+  the ABI a property of RcppParallel. `TBB_LIB` / `TBB_INC` are still honoured
+  for anyone supplying their own build. This requires cmake, which Rtools has
+  shipped since Rtools42; toolchains without one continue to use the tinythread
+  fallback.
+
+* As a consequence, `RcppParallel::RcppParallelLibs()` now emits `-ltbb` and
+  `-ltbbmalloc` on Windows, in addition to `-lRcppParallel` (which remains
+  necessary there for the entry points RcppParallel compiles itself, such as
+  `isProcessForkedChild`). Packages that previously resolved TBB symbols out of
+  `RcppParallel.dll`, or the tbbmalloc API via `-lRcppParallel` alone, should
+  rebuild against these flags.
+
+* The `tbb.dll` compatibility stub is gone. It existed to publish the
+  pre-oneTBB `task_scheduler_observer` entry point on top of a statically
+  linked runtime, but because it linked the TBB archives itself it amounted to
+  a second, independent copy of the oneTBB scheduler living in the same
+  process. That entry point is now exported by the real `tbb.dll`, as it
+  already was by the shared libraries on other platforms, so binaries built
+  against RcppParallel 5.1.11 and earlier continue to resolve it.
+
+* Fixed `RcppParallel::tbbLibraryPath()` returning `NULL` on Windows, and
+  `tbbRoot()` reporting a directory that need not exist on the machine running
+  the package -- for a pre-built binary, the Rtools tree of the machine that
+  built it. Both now describe the installation actually in use. (#270)
 
 * Fixed an issue where compiling code including `tbb/parallel_for_each.h`
   could fail with toolchains that accept `-std=c++20` but provide a

R/tbb.R

@@ -22,13 +22,9 @@ tbbLibraryPath <- function(name = NULL) {
       return(tbbRoot)
 
    # form library names
-   #
-   # on Windows the library we install is the 'tbb.dll' stub -- TBB itself is
-   # inside RcppParallel.dll -- so look for that first. the static archives are
-   # still tried afterwards, for a TBB_LIB pointed at an Rtools tree at runtime
    tbbLibNames <- list(
       "Darwin"  = paste0("lib", name, ".dylib"),
-      "Windows" = c(paste0(name, ".dll"), paste0("lib", name, c("12", ""), ".a")),
+      "Windows" = paste0(name, ".dll"),
       "SunOS"   = paste0("lib", name, ".so"),
       "Linux"   = paste0("lib", name, c(".so.2", ".so"))
    )
@@ -89,35 +85,35 @@ tbbCxxFlags <- function() {
 # Return the linker flags required for TBB on this platform
 tbbLdFlags <- function() {
 
-   # on Windows, we statically link to oneTBB
+   # on Windows every symbol must be resolved at link time -- there is no
+   # equivalent of lazy binding or '-undefined dynamic_lookup' -- so a
+   # downstream package needs both the TBB libraries it calls into and
+   # RcppParallel itself, for the entry points we compile (e.g.
+   # isProcessForkedChild). elsewhere those resolve from the process at load
+   # time and only TBB needs naming
    if (is_windows()) {
 
-      libPath <- archSystemFile("libs")
-
-      ldFlags <- sprintf("-L%s -lRcppParallel", asBuildPath(libPath))
-
-      # 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.
-      #
-      # 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)))
+      libsPath <- archSystemFile("libs")
+      ldFlags <- sprintf("-L%s -lRcppParallel", asBuildPath(libsPath))
+
+      # only name the TBB libraries when there are TBB libraries to name: a
+      # build that fell back to tinythread ships none, and asking the linker
+      # for them would fail the downstream build outright
+      if (TBB_ENABLED && !is.null(tbbLibraryPath("tbb"))) {
+         fmt <- "%s -L%s -l%s -l%s"
+         ldFlags <- sprintf(
+            fmt,
+            ldFlags,
+            asBuildPath(tbbLibraryPath()),
+            TBB_NAME,
+            TBB_MALLOC_NAME
+         )
+      }
 
       return(ldFlags)
 
    }
-   
+
    # shortcut if TBB_LIB defined
    tbbLib <- Sys.getenv("TBB_LINK_LIB", Sys.getenv("TBB_LIB", unset = TBB_LIB))
    if (nzchar(tbbLib)) {
@@ -150,15 +146,6 @@ tbbLdFlags <- function() {
 
 tbbRoot <- function() {
 
-   # on Windows, always answer with our own library directory. TBB is linked
-   # statically into RcppParallel.dll there, and the only TBB library we
-   # install is the tbb.dll stub, so that directory is the whole story. TBB_LIB
-   # is worse than useless here: it records the Rtools tree of whichever
-   # machine ran configure, which for a pre-built binary is a path that need
-   # not exist on the user's machine at all (#270)
-   if (is_windows())
-      return(archSystemFile("lib"))
-
    if (nzchar(TBB_LIB))
       return(TBB_LIB)