build the bundled onetbb as a shared library on windows (#274)

* 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.

* fix the shared tbb link on rtools42 and aarch64 windows

Two failures from the first CI run, both only reachable now that TBB is linked
as a shared library on Windows -- a static build has no link step.

Rtools42 (gcc 10): the TBB link failed with undefined references to
__stack_chk_fail, __strncpy_chk and __strncat_chk. TBB compiles with
-fstack-protector-strong and _FORTIFY_SOURCE, and on mingw those helpers live
in libssp rather than libgcc; newer toolchains pull it in implicitly, Rtools42
does not. Pass -lssp via CMAKE_SHARED_LINKER_FLAGS, mirroring the -lssp that
configure.R already adds for RcppParallel's own link.

aarch64 Windows: 'lld: error: unknown argument: -z'. Rtools45-aarch64 drives
clang with lld, and Clang.cmake adds -Wl,-z,relro,-z,now for any non-Apple
target. GNU.cmake already guards this with NOT MINGW; give Clang.cmake the
equivalent guard.

windows-latest (release) passed on the first run, so the shared mingw build,
the PREFIX "" naming and the reordered load in .onLoad() all work.

* link libssp via TBB_COMMON_LINK_LIBS rather than the shared linker flags

TBB compiles with -fstack-protector-strong and _FORTIFY_SOURCE=2, and on
mingw the helpers those need (__stack_chk_fail, __stack_chk_guard,
__strncpy_chk, ...) live in libssp rather than libgcc. Rtools42 (gcc 10)
does not pull it in implicitly, so the link fails with undefined
references to them.

Passing -lssp through CMAKE_SHARED_LINKER_FLAGS was not enough: those
flags are emitted before the objects, and GNU ld only resolves symbols
that are already undefined when it reaches a library, so it was
discarded. tbb and tbbmalloc happened to survive that because
tbb_handle_ipo gives them LTO; tbbmalloc_proxy has no such call and
failed to link. Putting ssp in TBB_COMMON_LINK_LIBS routes it through
target_link_libraries instead, which lands after the objects, and covers
all three targets.

* fix the RcppParallel.dll half of the downstream import check

Two reasons it was never actually running, both visible in the CI log
even though the job passed:

- the path was built with paste0("libs", .Platform$r_arch), giving
  'libsx64' rather than 'libs/x64'. system.file() returned "" for that,
  so objdump was handed '/RcppParallel.dll' and reported "No such file".
  Use archSystemFile(), which the package already has for this, and fail
  loudly if the library still can't be found.

- on aarch64 the runner has an x86_64 objdump from C:/mingw64 ahead of
  Rtools' own on the PATH, and it cannot read an aarch64 PE, so both
  halves of the check skipped there. Try the objdump sitting beside the
  compiler first, and fall back through the remaining candidates.

* locate objdump via the compiler R actually builds with

The previous attempt looked beside Sys.which("gcc"/"clang"), but on the
aarch64 runner those resolve to C:/mingw64 -- the same x86_64 toolchain
that leads the PATH -- so both candidates were the one objdump that
cannot read an aarch64 PE, and the check still skipped there. They were
also not deduplicated, differing only in slash direction.

'R CMD config CC' names the compiler that produced the library, so its
objdump can read it by construction. Try that first, and normalize the
candidates so one objdump is not tried under two spellings.

* don't emit -ltbb when no tbb was installed

The Windows branch of tbbLdFlags() named the TBB libraries
unconditionally, dropping the file.exists() guard the previous stub-based
code had. On a build that fell back to tinythread -- configure.R sets
TBB_ENABLED = FALSE only there, when cmake is missing and TBB_LIB is
unset -- RcppParallelLibs() then emitted

    -L<libs> -lRcppParallel -L<lib> -ltbb -ltbbmalloc

for libraries that were never built or shipped, so a downstream package
would fail to link with "cannot find -ltbb". CxxFlags() correctly reports
-DRCPP_PARALLEL_USE_TBB=0 in that configuration, so the package is not
even using TBB.

Name them only when TBB is enabled and the library actually resolves.

Also drop an unverified aside from NEWS: Rtools42 and Rtools45 are
confirmed to ship cmake, Rtools40 was not checked.

Author: kevinushey

.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)