From 05db5073f58e43f7cabe1132b7c518172800e28c Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Mon, 27 Jul 2026 21:08:55 -0700 Subject: [PATCH 1/2] 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 --- .github/scripts/tbb-downstream-check.R | 41 +++++++++++++- .github/workflows/R-CMD-check.yaml | 19 ++++--- .github/workflows/rstan.yaml | 6 ++- NEWS.md | 4 +- R/tbb.R | 11 +++- patches/legacy_tbb_abi.diff | 1 - patches/mingw_gnu_cmake.diff | 28 +++++----- src/install.libs.R | 39 +++++++++----- src/tbb/cmake/compilers/GNU.cmake | 22 ++++---- src/tbb/src/tbb/observer_proxy.cpp | 2 +- tools/config/configure.R | 75 ++++++++++++++++++-------- 11 files changed, 174 insertions(+), 74 deletions(-) diff --git a/.github/scripts/tbb-downstream-check.R b/.github/scripts/tbb-downstream-check.R index 517bdd84..6b6cd36f 100644 --- a/.github/scripts/tbb-downstream-check.R +++ b/.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") diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml index 37a47702..06216894 100644 --- a/.github/workflows/R-CMD-check.yaml +++ b/.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 diff --git a/.github/workflows/rstan.yaml b/.github/workflows/rstan.yaml index a598e1d1..4d0f76a0 100644 --- a/.github/workflows/rstan.yaml +++ b/.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: | diff --git a/NEWS.md b/NEWS.md index 3a43c7de..7af01545 100644 --- a/NEWS.md +++ b/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 diff --git a/R/tbb.R b/R/tbb.R index 55437c8c..e17b0477 100644 --- a/R/tbb.R +++ b/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))) diff --git a/patches/legacy_tbb_abi.diff b/patches/legacy_tbb_abi.diff index 9cef9c80..082ceba1 100644 --- a/patches/legacy_tbb_abi.diff +++ b/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 @@ diff --git a/patches/mingw_gnu_cmake.diff b/patches/mingw_gnu_cmake.diff index d0a4e541..cd2ff607 100644 --- a/patches/mingw_gnu_cmake.diff +++ b/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 $<$>:-flifetime-dse=1>) diff --git a/src/install.libs.R b/src/install.libs.R index 6ea15481..6381b470 100644 --- a/src/install.libs.R +++ b/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 { diff --git a/src/tbb/cmake/compilers/GNU.cmake b/src/tbb/cmake/compilers/GNU.cmake index bc86acc1..8f9f0b59 100644 --- a/src/tbb/cmake/compilers/GNU.cmake +++ b/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}") diff --git a/src/tbb/src/tbb/observer_proxy.cpp b/src/tbb/src/tbb/observer_proxy.cpp index 90389f66..07254196 100644 --- a/src/tbb/src/tbb/observer_proxy.cpp +++ b/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 */ \ No newline at end of file +#endif /* _WIN32 */ diff --git a/tools/config/configure.R b/tools/config/configure.R index 03c07c88..0d6b8c43 100644 --- a/tools/config/configure.R +++ b/tools/config/configure.R @@ -34,12 +34,18 @@ if (file.exists(makevars)) { # release still gates behind TBB_PREVIEW_TASK_ISOLATION, and its library # doesn't export isolate_within_arena either, so there is nothing to link # against even if the declaration were forced into view. we build the -# bundled copy of oneTBB in that case instead +# bundled copy of oneTBB in that case instead -- unless the user pointed +# TBB_LIB somewhere themselves, which is honoured as-is if (.Platform$OS.type == "windows") { gccPath <- normalizePath(Sys.which("gcc"), winslash = "/") tbbLib <- Sys.getenv("TBB_LIB", unset = NA) + + # a TBB the user configured explicitly is honoured whatever its vintage; + # only one discovered next to gcc has to pass the oneTBB check below + tbbLibConfigured <- !is.na(tbbLib) + if (is.na(tbbLib)) tbbLib <- normalizePath(file.path(gccPath, "../../lib"), winslash = "/") @@ -47,8 +53,11 @@ if (.Platform$OS.type == "windows") { if (is.na(tbbInc)) tbbInc <- normalizePath(file.path(gccPath, "../../include"), winslash = "/") + # whenever we do adopt this tree, the library names have to come with it: + # a legacy TBB spells its archives 'libtbb_static.a' and + # 'libtbbmalloc_static.a', which the defaults below would not find tbbFiles <- list.files(tbbLib, pattern = "^libtbb") - if (length(tbbFiles) && file.exists(file.path(tbbInc, "oneapi"))) { + if (length(tbbFiles) && (tbbLibConfigured || file.exists(file.path(tbbInc, "oneapi")))) { tbbPattern <- "^lib(tbb\\d*(?:_static)?)\\.a$" tbbName <- grep(tbbPattern, tbbFiles, perl = TRUE, value = TRUE) @@ -146,11 +155,12 @@ if (tryAutoDetect) { } # if we didn't find a TBB to use, we'll build the bundled copy from -# sources -- which requires cmake. on Windows, a missing or too-old cmake -# is not fatal: toolchains that can't build oneTBB (e.g. Rtools40, which -# provides neither cmake nor a TBB) fall back to tinythread instead +# sources -- which requires cmake. on Windows, a cmake that is missing, +# unusable or too old is not fatal: toolchains that can't build oneTBB +# (e.g. Rtools40, which provides neither cmake nor a TBB) fall back to +# tinythread instead define(CMAKE = "") -useBundledTbb <- FALSE +buildBundledTbb <- FALSE if (is.na(tbbLib)) { @@ -177,32 +187,51 @@ if (is.na(tbbLib)) { # make sure we have an appropriate version of cmake installed # (use the resolved path; cmake may not be on the PATH) - cmakeVersion <- if (!is.na(cmake)) { - output <- system(paste(shQuote(cmake), "--version"), intern = TRUE)[[1L]] - numeric_version(sub("cmake version ", "", output)) + # + # any failure here -- a CMAKE pointing at nothing runnable, version output + # we can't parse -- is treated as 'no usable cmake' rather than allowed to + # propagate, so that Windows can still fall back to tinythread below + cmakeVersion <- NA + if (!is.na(cmake)) { + cmakeVersion <- tryCatch({ + output <- system(paste(shQuote(cmake), "--version"), intern = TRUE) + numeric_version(sub("cmake version ", "", output[[1L]])) + }, condition = function(cnd) NA) } - useBundledTbb <- !is.na(cmake) && cmakeVersion >= "3.5" + buildBundledTbb <- !is.na(cmakeVersion) && cmakeVersion >= "3.5" + + if (buildBundledTbb) { - if (useBundledTbb) { define(CMAKE = cmake) - } else if (.Platform$OS.type != "windows") { - if (is.na(cmake)) - stop("cmake was not found") - stop("error: RcppParallel requires cmake (>= 3.6); you have ", cmakeVersion) - } else if (is.na(cmake)) { - writeLines("cmake was not found; building RcppParallel without a tbb backend") + } else { - fmt <- "cmake %s is too old (need >= 3.5); building RcppParallel without a tbb backend" - writeLines(sprintf(fmt, cmakeVersion)) + + reason <- if (is.na(cmake)) { + "cmake was not found" + } else if (is.na(cmakeVersion)) { + sprintf("couldn't determine the version of the cmake at '%s'", cmake) + } else { + sprintf("cmake %s is too old (need >= 3.5)", cmakeVersion) + } + + # not fatal on Windows: toolchains that can build neither an oneTBB nor + # anything else (e.g. Rtools40, which provides no cmake and no TBB) use + # the tinythread backend instead + if (.Platform$OS.type == "windows") + writeLines(paste0(reason, "; building RcppParallel without a tbb backend")) + else + stop("error: RcppParallel requires cmake (>= 3.5); ", reason) + } } # oneTBB appends its binary version to the library name on Windows, so the # bundled build produces 'libtbb12.a' rather than 'libtbb.a' there -if (.Platform$OS.type == "windows" && useBundledTbb) - tbbName <- Sys.getenv("TBB_NAME", unset = "tbb12") +# (tbbmalloc gets no such suffix, so its default name still applies) +if (.Platform$OS.type == "windows" && buildBundledTbb && !nzchar(Sys.getenv("TBB_NAME"))) + tbbName <- "tbb12" # now, define TBB_LIB and TBB_INC as appropriate define( @@ -245,7 +274,7 @@ pkgLibs <- if (.Platform$OS.type == "windows") { "-l$(TBB_MALLOC_NAME)" ) - } else if (useBundledTbb) { + } else if (buildBundledTbb) { # the bundled oneTBB is built as a static library on Windows, so # RcppParallel.dll takes exactly the same shape as it does with an @@ -308,7 +337,7 @@ if (!is.na(tbbLib)) { } # PKG_CXXFLAGS -if (.Platform$OS.type == "windows" && is.na(tbbLib) && !useBundledTbb) { +if (.Platform$OS.type == "windows" && is.na(tbbLib) && !buildBundledTbb) { define(TBB_ENABLED = FALSE) define(PKG_CXXFLAGS = "-DRCPP_PARALLEL_USE_TBB=0") } else { From c075ab7c929bb72b152daa1933bfce1ed3078694 Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Mon, 27 Jul 2026 23:49:59 -0700 Subject: [PATCH 2/2] 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. --- .github/scripts/tbb-downstream-check.R | 73 +++++++++++++++++++------- R/tbb.R | 27 +++++----- 2 files changed, 67 insertions(+), 33 deletions(-) diff --git a/.github/scripts/tbb-downstream-check.R b/.github/scripts/tbb-downstream-check.R index 6b6cd36f..214d6df2 100644 --- a/.github/scripts/tbb-downstream-check.R +++ b/.github/scripts/tbb-downstream-check.R @@ -97,37 +97,72 @@ if (status != 0L) 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. +# 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 { + } else if (!readable) { - output <- system2(objdump, c("-p", shQuote(dllName)), stdout = TRUE, stderr = TRUE) + # 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) - # 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 = ", "))) + } else { - 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") + 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") + } } diff --git a/R/tbb.R b/R/tbb.R index e17b0477..673913d9 100644 --- a/R/tbb.R +++ b/R/tbb.R @@ -92,21 +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 import of 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. # - # 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 + # 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)))