Skip to content

Commit 4de2066

Browse files
authored
Merge branch 'master' into fix/linux-versioned-tbb-libs
2 parents 4a5a1fe + 8f0f0d9 commit 4de2066

5 files changed

Lines changed: 275 additions & 8 deletions

File tree

NEWS.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,12 @@
88
otherwise fail to load after an upgrade with "libtbb.so.2: cannot open shared
99
object file".
1010

11+
* Fixed linking of downstream packages using the TBB scalable allocator
12+
on Windows, e.g. via RcppArmadillo's `ARMA_USE_TBB_ALLOC`. RcppParallel
13+
now links the whole Rtools `tbbmalloc` archive into `RcppParallel.dll`
14+
and re-exports its API, so that `scalable_malloc`, `scalable_free`, and
15+
friends can be resolved by packages linking with `-lRcppParallel`.
16+
1117
* Fixed installation on Windows toolchains providing an older (non-oneTBB)
1218
copy of TBB, e.g. Rtools42: the tbb stub library is now built by
1319
re-exporting the static TBB library, rather than wrapping the oneTBB

src/install.libs.R

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -431,9 +431,13 @@ splitCompilerVar <- function(compilerVar, flagsVar) {
431431
return(FALSE)
432432

433433
tokens <- scan(text = compiler, what = character(), quiet = TRUE)
434-
if (length(tokens) < 2L)
434+
if (length(tokens) == 0L)
435435
return(FALSE)
436436

437+
# always re-set the compiler from the parsed tokens, even when there are
438+
# no trailing flags: scan() strips surrounding whitespace, so this also
439+
# normalizes values like ' g++' (produced when e.g. '$(CCACHE) g++'
440+
# expands with an empty CCACHE), which CMake would otherwise reject
437441
setenv(compilerVar, tokens[[1L]])
438442

439443
oldFlags <- Sys.getenv(flagsVar)

tests/scalable-allocator.R

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
2+
# Check that packages using TBB's scalable allocator can be compiled, linked,
3+
# and loaded against this installation of RcppParallel.
4+
#
5+
# On Windows, downstream packages link only '-lRcppParallel', and so this
6+
# requires RcppParallel.dll to re-export the tbbmalloc API on their behalf.
7+
# On other platforms, the symbols are left undefined at link time, and
8+
# resolved from the tbbmalloc library loaded when RcppParallel is loaded.
9+
#
10+
# https://github.com/RcppCore/RcppParallel/pull/262
11+
12+
library(RcppParallel)
13+
14+
# building the test package requires a toolchain, so keep this test
15+
# off of CRAN's machines
16+
ci <- nzchar(Sys.getenv("CI")) || identical(Sys.getenv("NOT_CRAN"), "true")
17+
if (!ci) {
18+
writeLines("Not running on CI; skipping scalable allocator test.")
19+
quit(save = "no")
20+
}
21+
22+
# the scalable allocator is only available with the TBB backend
23+
if (!RcppParallel:::TBB_ENABLED) {
24+
writeLines("TBB is not enabled; skipping scalable allocator test.")
25+
quit(save = "no")
26+
}
27+
28+
# generate the test package
29+
pkgRoot <- file.path(tempdir(), "scalabletest")
30+
dir.create(file.path(pkgRoot, "src"), recursive = TRUE, showWarnings = FALSE)
31+
32+
writeLines(con = file.path(pkgRoot, "DESCRIPTION"), c(
33+
"Package: scalabletest",
34+
"Type: Package",
35+
"Title: Test Linking to the TBB Scalable Allocator",
36+
"Version: 0.1.0",
37+
"Author: RcppParallel Authors",
38+
"Maintainer: RcppParallel Authors <noreply@example.com>",
39+
"Description: Confirms that packages can use the TBB scalable allocator.",
40+
"License: GPL-2",
41+
"Imports: RcppParallel"
42+
))
43+
44+
writeLines(con = file.path(pkgRoot, "NAMESPACE"), c(
45+
"useDynLib(scalabletest)",
46+
"importFrom(RcppParallel, RcppParallelLibs)"
47+
))
48+
49+
writeLines(con = file.path(pkgRoot, "src", "Makevars"), c(
50+
'PKG_CXXFLAGS = $(shell "${R_HOME}/bin/Rscript" -e "RcppParallel::CxxFlags()")',
51+
'PKG_LIBS = $(shell "${R_HOME}/bin/Rscript" -e "RcppParallel::RcppParallelLibs()")'
52+
))
53+
54+
writeLines(con = file.path(pkgRoot, "src", "Makevars.win"), c(
55+
'PKG_CXXFLAGS = $(shell "${R_HOME}/bin${R_ARCH_BIN}/Rscript.exe" -e "RcppParallel::CxxFlags()")',
56+
'PKG_LIBS = $(shell "${R_HOME}/bin${R_ARCH_BIN}/Rscript.exe" -e "RcppParallel::RcppParallelLibs()")'
57+
))
58+
59+
writeLines(con = file.path(pkgRoot, "src", "scalable.cpp"), c(
60+
"#include <tbb/scalable_allocator.h>",
61+
"",
62+
"#include <R.h>",
63+
"#include <Rinternals.h>",
64+
"",
65+
'extern "C" SEXP scalable_roundtrip(SEXP sizeSEXP)',
66+
"{",
67+
" int size = Rf_asInteger(sizeSEXP);",
68+
"",
69+
" double* data = (double*) scalable_malloc(size * sizeof(double));",
70+
" if (data == NULL)",
71+
" return Rf_ScalarLogical(FALSE);",
72+
"",
73+
" for (int i = 0; i < size; i++)",
74+
" data[i] = i;",
75+
"",
76+
" double total = 0.0;",
77+
" for (int i = 0; i < size; i++)",
78+
" total += data[i];",
79+
"",
80+
" scalable_free(data);",
81+
"",
82+
" double expected = (double) size * (size - 1) / 2;",
83+
" return Rf_ScalarLogical(total == expected);",
84+
"}"
85+
))
86+
87+
# install it, making sure child processes resolve this RcppParallel
88+
libDir <- file.path(tempdir(), "library")
89+
dir.create(libDir, recursive = TRUE, showWarnings = FALSE)
90+
Sys.setenv(R_LIBS = paste(.libPaths(), collapse = .Platform$path.sep))
91+
92+
# R CMD check sets R_TESTS=startup.Rs, which breaks R sub-processes run
93+
# with a different working directory -- like the Rscript invocations in
94+
# the test package's Makevars (see also tests/doRUnit.R)
95+
Sys.setenv(R_TESTS = "")
96+
97+
rExe <- file.path(R.home("bin"), if (.Platform$OS.type == "windows") "R.exe" else "R")
98+
args <- c("CMD", "INSTALL", "--no-multiarch", paste0("--library=", shQuote(libDir)), shQuote(pkgRoot))
99+
100+
output <- suppressWarnings(system2(rExe, args, stdout = TRUE, stderr = TRUE))
101+
writeLines(output)
102+
103+
status <- attr(output, "status")
104+
if (is.numeric(status) && status != 0L)
105+
stop("error installing test package (status code ", status, ")")
106+
107+
# load it, and check that the scalable allocator can be used
108+
invisible(loadNamespace("scalabletest", lib.loc = libDir))
109+
ok <- .Call("scalable_roundtrip", 1000L, PACKAGE = "scalabletest")
110+
writeLines(paste("scalable allocator round trip:", if (isTRUE(ok)) "OK" else "FAILED"))
111+
stopifnot(isTRUE(ok))

tests/testInstallLibs.R

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
# Unit tests for the install-time helpers defined in src/install.libs.R.
2+
#
3+
# These helpers run during 'R CMD INSTALL' (before the package is loadable),
4+
# so they live in src/install.libs.R rather than in R/ and cannot be reached
5+
# via library(RcppParallel). Instead, we locate that source file, evaluate the
6+
# helper definitions that precede its '# Main' section (evaluating the whole
7+
# file would trigger an actual install), and exercise them directly.
8+
9+
# locate src/install.libs.R by walking up from the working directory; this is
10+
# reachable when tests are run from the package sources. If it cannot be found
11+
# (e.g. only the installed package is available), skip rather than fail.
12+
findInstallLibs <- function() {
13+
dir <- normalizePath(getwd(), mustWork = FALSE)
14+
for (i in 1:8) {
15+
candidate <- file.path(dir, "src", "install.libs.R")
16+
if (file.exists(candidate))
17+
return(candidate)
18+
parent <- dirname(dir)
19+
if (identical(parent, dir))
20+
break
21+
dir <- parent
22+
}
23+
NA_character_
24+
}
25+
26+
path <- findInstallLibs()
27+
if (is.na(path)) {
28+
writeLines("skipping: could not locate src/install.libs.R")
29+
quit(save = "no", status = 0L)
30+
}
31+
32+
# evaluate only the helper definitions, stopping before the '# Main' section
33+
lines <- readLines(path)
34+
marker <- grep("^# Main", lines)
35+
if (length(marker))
36+
lines <- lines[seq_len(marker[[1L]] - 1L)]
37+
38+
env <- new.env(parent = globalenv())
39+
eval(parse(text = paste(lines, collapse = "\n")), envir = env)
40+
splitCompilerVar <- get("splitCompilerVar", envir = env)
41+
42+
# minimal assertion harness
43+
failures <- 0L
44+
check <- function(cond, label) {
45+
ok <- isTRUE(cond)
46+
if (!ok)
47+
failures <<- failures + 1L
48+
writeLines(sprintf("%s - %s", if (ok) "PASS" else "FAIL", label))
49+
}
50+
51+
# run 'expr' with the given environment variables set, restoring the previous
52+
# environment afterwards so tests can't leak into one another (or into CC/CXX)
53+
withEnv <- function(vars, expr) {
54+
keys <- names(vars)
55+
previous <- Sys.getenv(keys, unset = NA, names = TRUE)
56+
on.exit({
57+
set <- previous[!is.na(previous)]
58+
if (length(set))
59+
do.call(Sys.setenv, as.list(set))
60+
unset <- keys[is.na(previous)]
61+
if (length(unset))
62+
Sys.unsetenv(unset)
63+
}, add = TRUE)
64+
do.call(Sys.setenv, as.list(vars))
65+
force(expr)
66+
}
67+
68+
# the regression this branch fixes: '$(CCACHE) g++' with an empty CCACHE
69+
# expands to ' g++' (leading space). scan() yields a single token, and the
70+
# old early-return left the leading space in place, forwarding an invalid
71+
# '-DCMAKE_CXX_COMPILER= g++' to CMake. The compiler must be normalized.
72+
withEnv(c(TEST_CXX = " g++", TEST_CXXFLAGS = ""), {
73+
result <- splitCompilerVar("TEST_CXX", "TEST_CXXFLAGS")
74+
check(isTRUE(result), "leading-whitespace compiler returns TRUE")
75+
check(identical(Sys.getenv("TEST_CXX"), "g++"),
76+
"leading-whitespace compiler is normalized (no leading space)")
77+
check(identical(Sys.getenv("TEST_CXXFLAGS"), ""),
78+
"leading-whitespace compiler leaves flags untouched")
79+
})
80+
81+
# a plain single-token compiler is already clean, but should still be
82+
# re-set (and report TRUE) so the normalization path is exercised uniformly
83+
withEnv(c(TEST_CXX = "g++", TEST_CXXFLAGS = ""), {
84+
result <- splitCompilerVar("TEST_CXX", "TEST_CXXFLAGS")
85+
check(isTRUE(result), "single-token compiler returns TRUE")
86+
check(identical(Sys.getenv("TEST_CXX"), "g++"),
87+
"single-token compiler is preserved")
88+
})
89+
90+
# trailing tokens are split off as flags and prepended to any existing flags
91+
withEnv(c(TEST_CXX = "g++ -std=c++17 -O2", TEST_CXXFLAGS = "-Wall"), {
92+
result <- splitCompilerVar("TEST_CXX", "TEST_CXXFLAGS")
93+
check(isTRUE(result), "compiler with flags returns TRUE")
94+
check(identical(Sys.getenv("TEST_CXX"), "g++"),
95+
"compiler with flags splits off the compiler token")
96+
check(identical(Sys.getenv("TEST_CXXFLAGS"), "-std=c++17 -O2 -Wall"),
97+
"compiler with flags prepends split flags to existing flags")
98+
})
99+
100+
# a whitespace-only value tokenizes to nothing: report FALSE and change nothing
101+
withEnv(c(TEST_CXX = " ", TEST_CXXFLAGS = "-Wall"), {
102+
result <- splitCompilerVar("TEST_CXX", "TEST_CXXFLAGS")
103+
check(identical(result, FALSE), "whitespace-only compiler returns FALSE")
104+
check(identical(Sys.getenv("TEST_CXXFLAGS"), "-Wall"),
105+
"whitespace-only compiler leaves flags untouched")
106+
})
107+
108+
# an unset compiler variable is a no-op that reports FALSE
109+
Sys.unsetenv("TEST_CXX_UNSET")
110+
check(identical(splitCompilerVar("TEST_CXX_UNSET", "TEST_CXXFLAGS"), FALSE),
111+
"unset compiler variable returns FALSE")
112+
113+
if (failures > 0L)
114+
stop(sprintf("%d install.libs.R helper test(s) failed", failures))
115+
116+
writeLines("all install.libs.R helper tests passed")

tools/config/configure.R

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -224,19 +224,49 @@ define(
224224
)
225225

226226
# set PKG_LIBS
227-
pkgLibs <- if (!is.na(tbbLib)) {
228-
227+
pkgLibs <- if (.Platform$OS.type == "windows") {
228+
229+
if (!is.na(tbbLib) && file.exists(file.path(tbbInc, "oneapi"))) {
230+
231+
# downstream packages link with '-lRcppParallel' alone, so
232+
# RcppParallel.dll must provide the tbbmalloc API (scalable_malloc
233+
# and friends) even though RcppParallel itself never calls it; use
234+
# --whole-archive so those objects are linked in, and re-exported
235+
# via their '-export:' directives. tbbmalloc must precede tbb here:
236+
# both archives bundle an itt_notify object defining the same
237+
# symbols, and with tbbmalloc's copy already linked, tbb's is never
238+
# pulled in, avoiding duplicate definition errors
239+
c(
240+
"-Wl,-L\"$(TBB_LIB)\"",
241+
"-Wl,--whole-archive",
242+
"-l$(TBB_MALLOC_NAME)",
243+
"-Wl,--no-whole-archive",
244+
"-l$(TBB_NAME)"
245+
)
246+
247+
} else if (!is.na(tbbLib)) {
248+
249+
# with an older (non-oneTBB) toolchain like Rtools42, tbb and
250+
# tbbmalloc both define DllMain, so tbbmalloc cannot be linked
251+
# wholesale; its objects also carry no '-export:' directives, so
252+
# nothing would be re-exported anyhow -- just link as needed
253+
c(
254+
"-Wl,-L\"$(TBB_LIB)\"",
255+
"-l$(TBB_NAME)",
256+
"-l$(TBB_MALLOC_NAME)"
257+
)
258+
259+
}
260+
261+
} else if (!is.na(tbbLib)) {
262+
229263
c(
230264
"-Wl,-L\"$(TBB_LIB)\"",
231265
sprintf("-Wl,-rpath,%s", shQuote(tbbLib)),
232266
"-l$(TBB_NAME)",
233267
"-l$(TBB_MALLOC_NAME)"
234268
)
235-
236-
} else if (.Platform$OS.type == "windows") {
237-
238-
NULL
239-
269+
240270
} else if (R.version$os == "emscripten") {
241271

242272
c(

0 commit comments

Comments
 (0)