address review findings on the windows onetbb build (#272)

* address review findings on the windows onetbb build

- configure: derive TBB_NAME / TBB_MALLOC_NAME whenever an Rtools tree is
  adopted, not only when it is oneTBB. gating both on 'oneapi' broke an
  explicitly configured legacy TBB_LIB, which linked -ltbb against a
  directory holding libtbb_static.a
- configure: don't let the cmake probe throw. it now runs on Windows, where
  an unrunnable CMAKE or unparseable --version output has to degrade to
  tinythread rather than fail the configure
- configure: correct the required cmake version in the error message (3.5,
  matching the check and DESCRIPTION), and rename useBundledTbb to
  buildBundledTbb so it no longer collides with the install.libs.R function
- install.libs.R: report the Windows static build explicitly instead of
  logging 'no tbb runtime libraries found', which reads like a failure
- tbb.R: note that the stub carries its own oneTBB runtime, so resolving
  against it means a second scheduler rather than a free fallback
- downstream check: assert the built library imports nothing from the
  tbb.dll stub, which is the property that comment now relies on
- GNU.cmake: probe with an empty file rather than the null device, which
  drops the WIN32 special case and removes the unverified 'gcc -c NUL'
- add windows-11-arm to the downstream matrix, restore the trailing newline
  in observer_proxy.cpp, pin rstan.yaml against DESCRIPTION, checkout@v4
- regenerate patches/legacy_tbb_abi.diff and patches/mingw_gnu_cmake.diff

* 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

@@ -75,7 +75,7 @@ if (!RcppParallel:::TBB_ENABLED)
 makevars <- c(
    "CXX_STD = CXX17",
    sprintf("PKG_CPPFLAGS = -I\"%s\"", system.file("include", package = "RcppParallel")),
-   "PKG_CPPFLAGS += $(shell \"${R_HOME}/bin/Rscript\" -e \"RcppParallel::CxxFlags()\" | tail -n 1)",
+   "PKG_CPPFLAGS += $(shell \"${R_HOME}/bin${R_ARCH_BIN}/Rscript\" -e \"RcppParallel::CxxFlags()\" | tail -n 1)",
    "PKG_LIBS += $(shell \"${R_HOME}/bin${R_ARCH_BIN}/Rscript\" -e \"RcppParallel::RcppParallelLibs()\" | tail -n 1)"
 )
 
@@ -95,8 +95,80 @@ status <- system(paste(shQuote(file.path(R.home("bin"), "R")), "CMD SHLIB check.
 if (status != 0L)
    stop("downstream translation unit failed to build")
 
+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))
+
+   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 if (!readable) {
+
+      # 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)
+
+   } else {
+
+      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")
+
+   }
+
+}
+
 # loading also proves any load-time dependency on the tbb stub resolves
-dll <- dyn.load(paste0("check", .Platform$dynlib.ext))
+dll <- dyn.load(dllName)
 on.exit(dyn.unload(dll[["path"]]), add = TRUE)
 
 result <- .Call("tbb_downstream_check")

.github/workflows/R-CMD-check.yaml

@@ -31,7 +31,7 @@ jobs:
       GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
       R_KEEP_PKG_SOURCE: yes
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
 
       - uses: r-lib/actions/setup-r@v2
         with:
@@ -50,14 +50,21 @@ jobs:
   # depends on, and it is not covered by R CMD check: RcppParallel can install
   # perfectly well while leaving downstream packages unable to build.
   downstream:
-    runs-on: windows-latest
+    runs-on: ${{ matrix.config.os }}
 
-    name: downstream (${{ matrix.r }})
+    name: downstream (${{ matrix.config.os }}, ${{ matrix.config.r }})
 
     strategy:
       fail-fast: false
       matrix:
-        r: ['4.2', 'release']
+        config:
+          # Rtools42 has no oneTBB, so this one builds the bundled copy
+          - {os: windows-latest, r: '4.2'}
+          - {os: windows-latest, r: 'release'}
+          # RcppParallelLibs() changed for every Windows platform, and arm64
+          # has both its own vendored patch and itt_notify compiled out, so
+          # its export surface is the one most likely to differ
+          - {os: windows-11-arm, r: 'release'}
 
     env:
       GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
@@ -70,11 +77,11 @@ jobs:
         shell: bash
 
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
 
       - uses: r-lib/actions/setup-r@v2
         with:
-          r-version: ${{ matrix.r }}
+          r-version: ${{ matrix.config.r }}
           use-public-rspm: true
 
       - name: install RcppParallel

.github/workflows/rstan.yaml

@@ -49,7 +49,7 @@ jobs:
         shell: bash
 
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
 
       - uses: r-lib/actions/setup-r@v2
         with:
@@ -83,9 +83,11 @@ jobs:
         run: |
           Rscript -e 'install.packages(c("StanHeaders","rstan"), type = "source", dependencies = FALSE, INSTALL_opts = "--no-multiarch")'
 
+      # compare against this checkout's DESCRIPTION rather than looking for a
+      # '.9000' suffix, so this keeps working across development version bumps
       - name: check RcppParallel is still the one we built
         run: |
-          Rscript -e 'v <- as.character(packageVersion("RcppParallel")); writeLines(paste("RcppParallel", v)); if (!grepl("[.]9000$", v)) stop("RcppParallel was replaced by a released build; the rstan build did not test this branch")'
+          Rscript -e 'installed <- packageVersion("RcppParallel"); expected <- package_version(read.dcf("DESCRIPTION")[1L, "Version"]); writeLines(sprintf("RcppParallel %s (expected %s)", installed, expected)); if (installed != expected) stop("RcppParallel was replaced by a different build; the rstan build did not test this branch")'
 
       - name: load rstan
         run: |

NEWS.md

@@ -20,7 +20,9 @@
   2017, whose headers downstream packages cannot build against: StanHeaders
   uses `tbb::this_task_arena::isolate`, which that release still gates behind
   `TBB_PREVIEW_TASK_ISOLATION` and does not export from its library. As a
-  result, rstan could no longer be built on R 4.2 for Windows.
+  result, rstan could no longer be built on R 4.2 for Windows. A TBB
+  configured explicitly via `TBB_LIB` is still used as-is, whatever its
+  vintage.
 
 * On Windows, `RcppParallel::RcppParallelLibs()` once again offers the TBB
   stub library, so that packages taking all of their linker flags from it

R/tbb.R

@@ -92,14 +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 dependency on 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.
+      #
+      # 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)))

patches/legacy_tbb_abi.diff

@@ -170,7 +170,6 @@
 +} // namespace tbb
 +
 +#endif /* _WIN32 */
-\ No newline at end of file
 --- a/src/tbb/src/tbb/def/lin32-tbb.def
 +++ b/src/tbb/src/tbb/def/lin32-tbb.def
 @@ -17,6 +17,26 @@

patches/mingw_gnu_cmake.diff

@@ -1,30 +1,32 @@
 diff --git a/src/tbb/cmake/compilers/GNU.cmake b/src/tbb/cmake/compilers/GNU.cmake
-index cf6d8bdb..bc86acc1 100644
+index cf6d8bdb..8f9f0b59 100644
 --- a/src/tbb/cmake/compilers/GNU.cmake
 +++ b/src/tbb/cmake/compilers/GNU.cmake
-@@ -47,19 +47,42 @@ endif()
+@@ -47,19 +47,44 @@ endif()
  # >=2.31.1). Capturing the output in CMake can be done like below. The version
  # information is written to either stdout or stderr. To not make any
  # assumptions, both are captured.
 +
-+# The null device is spelled differently on Windows. A mingw build driven by
-+# a native Windows CMake (rather than cross-compiled from a POSIX host) has no
-+# '/dev/null', and the compiler would fail with "no input files".
-+if (WIN32)
-+    set(_tbb_null_device "NUL")
-+else()
-+    set(_tbb_null_device "/dev/null")
-+endif()
++# Probe with a real (empty) source file rather than the null device. A mingw
++# build driven by a native Windows CMake (rather than cross-compiled from a
++# POSIX host) has no '/dev/null', and the compiler would fail with "no input
++# files"; an actual file avoids having to spell the null device per platform.
++set(_tbb_asm_probe_source ${CMAKE_BINARY_DIR}/CMakeFiles/tbb_asm_probe.c)
++set(_tbb_asm_probe_object ${CMAKE_BINARY_DIR}/CMakeFiles/tbb_asm_probe.o)
++file(WRITE ${_tbb_asm_probe_source} "")
 +
  execute_process(
 -    COMMAND ${CMAKE_COMMAND} -E env "LANG=C" ${CMAKE_CXX_COMPILER} -xc -c /dev/null -Wa,-v -o/dev/null
-+    COMMAND ${CMAKE_COMMAND} -E env "LANG=C" ${CMAKE_CXX_COMPILER} -xc -c ${_tbb_null_device} -Wa,-v -o${_tbb_null_device}
++    COMMAND ${CMAKE_COMMAND} -E env "LANG=C" ${CMAKE_CXX_COMPILER} -xc -c ${_tbb_asm_probe_source} -Wa,-v -o ${_tbb_asm_probe_object}
      OUTPUT_VARIABLE ASSEMBLER_VERSION_LINE_OUT
      ERROR_VARIABLE ASSEMBLER_VERSION_LINE_ERR
      OUTPUT_STRIP_TRAILING_WHITESPACE
      ERROR_STRIP_TRAILING_WHITESPACE
  )
-+unset(_tbb_null_device)
++
++file(REMOVE ${_tbb_asm_probe_source} ${_tbb_asm_probe_object})
++unset(_tbb_asm_probe_source)
++unset(_tbb_asm_probe_object)
  set(ASSEMBLER_VERSION_LINE ${ASSEMBLER_VERSION_LINE_OUT}${ASSEMBLER_VERSION_LINE_ERR})
  string(REGEX REPLACE ".*GNU assembler version ([0-9]+)\\.([0-9]+).*" "\\1" _tbb_gnu_asm_major_version "${ASSEMBLER_VERSION_LINE}")
  string(REGEX REPLACE ".*GNU assembler version ([0-9]+)\\.([0-9]+).*" "\\2" _tbb_gnu_asm_minor_version "${ASSEMBLER_VERSION_LINE}")
@@ -46,7 +48,7 @@ index cf6d8bdb..bc86acc1 100644
  message(TRACE "Extracted GNU assembler version: major=${_tbb_gnu_asm_major_version} minor=${_tbb_gnu_asm_minor_version}")
 
  math(EXPR _tbb_gnu_asm_version_number  "${_tbb_gnu_asm_major_version} * 1000 + ${_tbb_gnu_asm_minor_version}")
-@@ -81,7 +104,14 @@ endif()
+@@ -81,7 +106,14 @@ endif()
  if (NOT ${CMAKE_CXX_COMPILER_ID} STREQUAL Intel)
      # gcc 6.0 and later have -flifetime-dse option that controls elimination of stores done outside the object lifetime
      set(TBB_DSE_FLAG $<$<NOT:$<VERSION_LESS:${CMAKE_CXX_COMPILER_VERSION},6.0>>:-flifetime-dse=1>)

src/install.libs.R

@@ -39,21 +39,34 @@
 
       # using bundled TBB
       tbbSrc <- "tbb/build/lib_release"
-      tbbLibs <- list.files(
-         path       = tbbSrc,
-         pattern    = shlibPattern,
-         full.names = TRUE
-      )
 
-      logTbbLibraries(tbbLibs, tbbSrc)
-      for (tbbLib in tbbLibs) {
-         system2("cp", c("-P", shQuote(tbbLib), shQuote(tbbDest)))
-      }
+      # on Windows the bundled TBB was built as a static library, and is
+      # already linked into (and re-exported from) RcppParallel.dll; say so
+      # explicitly, rather than reporting that no runtime libraries were
+      # found -- there are none to find, and that reads like a failure
+      if (.Platform$OS.type == "windows") {
+
+         writeLines("** tbb is statically linked into RcppParallel.dll; no runtime libraries to install")
+
+      } else {
 
-      # restore the versioned library layout shipped by RcppParallel 5.1.11
-      # and earlier (a real 'libtbb.so.2' plus a 'libtbb.so' symlink)
-      if (Sys.info()[["sysname"]] == "Linux")
-         versionBundledTbbLibraries(tbbDest)
+         tbbLibs <- list.files(
+            path       = tbbSrc,
+            pattern    = shlibPattern,
+            full.names = TRUE
+         )
+
+         logTbbLibraries(tbbLibs, tbbSrc)
+         for (tbbLib in tbbLibs) {
+            system2("cp", c("-P", shQuote(tbbLib), shQuote(tbbDest)))
+         }
+
+         # restore the versioned library layout shipped by RcppParallel 5.1.11
+         # and earlier (a real 'libtbb.so.2' plus a 'libtbb.so' symlink)
+         if (Sys.info()[["sysname"]] == "Linux")
+            versionBundledTbbLibraries(tbbDest)
+
+      }
 
    } else {
 

src/tbb/cmake/compilers/GNU.cmake

@@ -48,23 +48,25 @@ endif()
 # information is written to either stdout or stderr. To not make any
 # assumptions, both are captured.
 
-# The null device is spelled differently on Windows. A mingw build driven by
-# a native Windows CMake (rather than cross-compiled from a POSIX host) has no
-# '/dev/null', and the compiler would fail with "no input files".
-if (WIN32)
-    set(_tbb_null_device "NUL")
-else()
-    set(_tbb_null_device "/dev/null")
-endif()
+# Probe with a real (empty) source file rather than the null device. A mingw
+# build driven by a native Windows CMake (rather than cross-compiled from a
+# POSIX host) has no '/dev/null', and the compiler would fail with "no input
+# files"; an actual file avoids having to spell the null device per platform.
+set(_tbb_asm_probe_source ${CMAKE_BINARY_DIR}/CMakeFiles/tbb_asm_probe.c)
+set(_tbb_asm_probe_object ${CMAKE_BINARY_DIR}/CMakeFiles/tbb_asm_probe.o)
+file(WRITE ${_tbb_asm_probe_source} "")
 
 execute_process(
-    COMMAND ${CMAKE_COMMAND} -E env "LANG=C" ${CMAKE_CXX_COMPILER} -xc -c ${_tbb_null_device} -Wa,-v -o${_tbb_null_device}
+    COMMAND ${CMAKE_COMMAND} -E env "LANG=C" ${CMAKE_CXX_COMPILER} -xc -c ${_tbb_asm_probe_source} -Wa,-v -o ${_tbb_asm_probe_object}
     OUTPUT_VARIABLE ASSEMBLER_VERSION_LINE_OUT
     ERROR_VARIABLE ASSEMBLER_VERSION_LINE_ERR
     OUTPUT_STRIP_TRAILING_WHITESPACE
     ERROR_STRIP_TRAILING_WHITESPACE
 )
-unset(_tbb_null_device)
+
+file(REMOVE ${_tbb_asm_probe_source} ${_tbb_asm_probe_object})
+unset(_tbb_asm_probe_source)
+unset(_tbb_asm_probe_object)
 set(ASSEMBLER_VERSION_LINE ${ASSEMBLER_VERSION_LINE_OUT}${ASSEMBLER_VERSION_LINE_ERR})
 string(REGEX REPLACE ".*GNU assembler version ([0-9]+)\\.([0-9]+).*" "\\1" _tbb_gnu_asm_major_version "${ASSEMBLER_VERSION_LINE}")
 string(REGEX REPLACE ".*GNU assembler version ([0-9]+)\\.([0-9]+).*" "\\2" _tbb_gnu_asm_minor_version "${ASSEMBLER_VERSION_LINE}")

src/tbb/src/tbb/observer_proxy.cpp

@@ -336,4 +336,4 @@ void __TBB_EXPORTED_FUNC task_scheduler_observer_v3::observe( bool enable ) {
 } // namespace internal
 } // namespace tbb
 
-#endif /* _WIN32 */
+#endif /* _WIN32 */