build the bundled onetbb as a shared library on windows

Windows was the only platform where TBB was not a shared library loaded at
runtime, and nearly every Windows-specific wart traced back to that. Use the
bundled oneTBB there too, built shared, shipped as tbb.dll / tbbmalloc.dll and
linked exactly as on other platforms.

Rtools ships static libraries only, so adopting its TBB meant linking it into
RcppParallel.dll. That made the TBB version -- and ABI -- a property of the
user's toolchain (Intel TBB 2017 on Rtools42, oneTBB later), and left
downstream packages with no TBB library to link against. The workarounds
accumulated: re-exporting the whole tbbmalloc archive so the allocator could be
resolved (#262), a tbb.dll stub so packages linking -ltbb could load (#249,
#250), building that stub differently for legacy toolchains (#258), offering it
from RcppParallelLibs() so rstan could link at all (#269), and a library lookup
that searched for archives never installed with the package (#270).

Worse, the stub linked the TBB archives itself, so it was a second complete
copy of the oneTBB scheduler. Both it and RcppParallel.dll export the same 81
tbb::detail::r1:: entry points, both are loaded in every session, and which one
a downstream package binds to is up to the linker. Two schedulers with separate
thread pools and separate global state is a correctness problem, not a linking
inconvenience.

This removes:

- the Rtools TBB detection in configure.R, and with it the TBB_NAME derivation
  from archive names and the tbb12 special case
- the --whole-archive tbbmalloc re-export, which only existed because there was
  no DLL to link
- BUILD_SHARED_LIBS=0 for Windows
- src/tbb-compat/ and buildTbbStub() entirely, along with the #ifndef _WIN32
  guard in observer_proxy.cpp and the __TBB_LEGACY_..._PROVIDED macro that let
  the stub tell which headers it was compiling against
- the Windows branches in .install.libs(), loadTbbLibrary(), tbbLdFlags() and
  tbbLibraryPath(), and the Windows-first load order in .onLoad()

Notes on the pieces that are not just deletions:

- mingw CMake would name a shared build libtbb.dll; patch PREFIX to "" so the
  runtime is tbb.dll, the name binaries built against 5.1.11 and earlier
  import. The import library keeps its lib prefix, so -ltbb still resolves.
- the legacy task_scheduler_observer_v3::observe definition now applies on
  Windows too. mingw has no .def file, so its export comes from a dllexport
  directive: the declaration gains TBB_EXPORT, which is empty when consuming
  the headers.
- .onLoad loads tbb before RcppParallel on every platform now, since
  RcppParallel.dll imports from tbb.dll and the package library directory is
  not on the DLL search path.
- tbbLdFlags() keeps -lRcppParallel on Windows: isProcessForkedChild is
  compiled into RcppParallel.dll, and Windows has no lazy binding to resolve it
  at load time.

The downstream CI check now asserts the property this is all for: the
downstream library's TBB symbols come from exactly one module, that module is
tbb.dll, and RcppParallel.dll imports from it rather than carrying its own copy.

Author: kevinushey

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

@@ -97,24 +97,27 @@ 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") {
+# 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.
+tbbImports <- function(dll) {
 
    objdump <- Sys.which("objdump")
-   output <- if (nzchar(objdump))
-      suppressWarnings(system2(objdump, c("-p", shQuote(dllName)), stdout = TRUE, stderr = TRUE))
+   if (!nzchar(objdump)) {
+      writeLines("** objdump not found; skipping the import table check")
+      return(NULL)
+   }
 
+   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 this
+   # 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
@@ -125,49 +128,71 @@ if (.Platform$OS.type == "windows") {
    # 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)) {
+   # 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)
+   if (!length(starts) || (is.numeric(status) && status != 0L)) {
+      fmt <- "** '%s' could not read the import table of '%s'; skipping the check"
+      writeLines(sprintf(fmt, objdump, basename(dll)))
+      writeLines(output)
+      return(NULL)
+   }
 
-      writeLines("** objdump not found; skipping the import table check")
+   imported <- sub(".*DLL Name:[[:space:]]*", "", output[starts])
+   ends <- c(starts[-1L], length(output) + 1L)
 
-   } else if (!readable) {
+   # 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]])
+   }
 
-      # 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)
+   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 <- file.path(
+      system.file(paste0("libs", .Platform$r_arch), package = "RcppParallel"),
+      "RcppParallel.dll"
+   )
+   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
+  provided since Rtools42; toolchains without it (e.g. Rtools40) 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,28 @@ 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)))
-
-      return(ldFlags)
+      libsPath <- archSystemFile("libs")
+      tbbPath <- tbbLibraryPath()
+
+      fmt <- "-L%s -lRcppParallel -L%s -l%s -l%s"
+      return(sprintf(
+         fmt,
+         asBuildPath(libsPath),
+         asBuildPath(tbbPath),
+         TBB_NAME,
+         TBB_MALLOC_NAME
+      ))
 
    }
-   
+
    # shortcut if TBB_LIB defined
    tbbLib <- Sys.getenv("TBB_LINK_LIB", Sys.getenv("TBB_LIB", unset = TBB_LIB))
    if (nzchar(tbbLib)) {
@@ -150,15 +139,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)
 

R/zzz.R

@@ -12,29 +12,6 @@
 
 loadTbbLibrary <- function(name) {
 
-   # On Windows, TBB is statically linked into RcppParallel.dll, but we
-   # still ship a stub tbb.dll for compatibility with packages linking via
-   # '-ltbb' (e.g. through StanHeaders). Such packages record a load-time
-   # dependency on 'tbb.dll', which the Windows loader can only resolve if
-   # we've already loaded it -- the library directory itself is not on the
-   # DLL search path.
-   if (is_windows()) {
-
-      # NOTE: resolve against the package's own library directory, where
-      # install.libs.R places the stub. this is what tbbRoot() reports on
-      # Windows too; spell it out here so that loading during .onLoad does
-      # not depend on the configured TBB_LIB in any way
-      path <- archSystemFile("lib", paste0(name, ".dll"))
-      if (!file.exists(path)) {
-         verboseMessage("tbb library '%s' not found in package 'lib' folder; skipping", name)
-         return(NULL)
-      }
-
-      verboseMessage("loading tbb library '%s'", path)
-      return(dyn.load(path, local = FALSE, now = TRUE))
-
-   }
-
    path <- tbbLibraryPath(name)
    if (is.null(path)) {
       verboseMessage("tbb library '%s' could not be resolved; skipping", name)
@@ -52,26 +29,21 @@ loadTbbLibrary <- function(name) {
 }
 
 .onLoad <- function(libname, pkgname) {
-   
-   # on Windows, load RcppParallel first
-   if (.Platform$OS.type == "windows") {
-      .dllInfo <<- library.dynam("RcppParallel", pkgname, libname)
-   }
-   
-   # load tbb, tbbmalloc
+
+   # load tbb, tbbmalloc first: RcppParallel links against them, and on Windows
+   # in particular the loader can only satisfy that dependency once they are in
+   # the process -- the package's library directory is not on the DLL search
+   # path, so it cannot find them by itself
    .tbbDllInfo       <<- loadTbbLibrary("tbb")
    .tbbMallocDllInfo <<- loadTbbLibrary("tbbmalloc")
-   
+
    # load tbbmalloc_proxy, but only if requested
    useTbbMallocProxy <- Sys.getenv("RCPP_PARALLEL_USE_TBBMALLOC_PROXY", unset = "FALSE")
    if (useTbbMallocProxy %in% c("TRUE", "True", "true", "1"))
       .tbbMallocProxyDllInfo <<- loadTbbLibrary("tbbmalloc_proxy")
-   
-   # load RcppParallel library if available
-   if (.Platform$OS.type != "windows") {
-      .dllInfo <<- library.dynam("RcppParallel", pkgname, libname, local = FALSE)
-   }
-   
+
+   .dllInfo <<- library.dynam("RcppParallel", pkgname, libname, local = FALSE)
+
 }
 
 .onUnload <- function(libpath) {