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

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,45 @@ 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)
+
+# 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.
+if (.Platform$OS.type == "windows") {
+
+   objdump <- Sys.which("objdump")
+   if (!nzchar(objdump)) {
+
+      writeLines("** objdump not found; skipping the import table check")
+
+   } else {
+
+      output <- system2(objdump, c("-p", shQuote(dllName)), stdout = TRUE, stderr = TRUE)
+
+      # 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 = ", ")))
+
+      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")
+      }
+
+   }
+
+}
+
 # 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

@@ -12,7 +12,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

@@ -98,8 +98,15 @@ tbbLdFlags <- function() {
       # 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
+      # still win, and no import of the stub is recorded unless something
+      # actually needs it.
+      #
+      # 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
       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 */