address review findings on the windows onetbb build

.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

@@ -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

@@ -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 */