Set dll exports - #1863
Set dll exports#1863MichaelChirico wants to merge 1 commit into
Conversation
|
So the presence of the .def file allows proper visibility handling (e.g. hidden by default except for explicit exports) without setting the visibility flags? |
|
Per Ivan's comment, it seems the visibility flags don't WAI on Windows? Rdatatable/data.table#7607 (comment) I also don't have a Windows machine to test, but if you have one, examine |
|
Interesting! But as I understand things, it doesn't really matter which symbols are exported in the package DLL for these reasons:
So for simplicity's sake I think we can safely ignore the win.def file here, even though R supports it? |
|
Yea, I think there's really not any issue with omitting the win.def file. It's more of a "doesn't hurt, and makes the .dll closer to the .so in intent" change. I combed the r-package-devel and r-devel archives and only find two references to win.def:
I don't think I 100% understand, but it looks like the former requires win.def to prevent 'illegal' names from being exported, and the latter is about a manually-populated .def being done incorrectly. I will try and ask on r-devel about this if I find the time :) |
|
Unless we find a good practical reason to do it, I'd lean towards not adopting this workflow. This way it's one less thing to know about, implement support for in usethis, maintain, etc. What do you think? |
|
That's my own instinct as well. I'd like to understand better why other "bedrock" packages like {Matrix} do it, though. Maybe we're missing something subtle. |
|
Threw this to Gemini for a "neutral" (ish) third opinion, and agreeing to close. The benefits are minimal. Here is a breakdown of the technical context, the history behind Executive Summary
1. Windows PE/COFF vs. Linux ELF Dynamic LinkingThe core difference between Unix shared objects (
Because R packages on Windows do not link against each other at link time via DLL import tables, having internal C functions exported in 2. How R Builds Windows DLLs (
|
| Consideration | With rlang-win.def |
Without rlang-win.def |
|---|---|---|
R-level safety (.Call) |
Protected via R_useDynamicSymbols(dll, FALSE) |
Protected via R_useDynamicSymbols(dll, FALSE) |
C Callable API (R_RegisterCCallable) |
Fully functional (uses R-internal registry in R.dll, not PE exports) |
Fully functional |
| Accidental collision risk | None | None (PE two-level namespace) |
| DLL Size | Shaves a few KB of string names from .edata |
Negligible difference |
| Maintenance Burden | Need to remember to update / maintain .def files across repos |
Zero extra maintenance |
Recommended Response for the PR
You can reply along the following lines:
- Windows dynamic linking is safe by design: Because Windows uses explicit, two-level import/export tables rather than ELF's flat symbol scope, un-hidden C symbols in
rlang.dllcannot cause runtime symbol interposition or collisions with other packages.- R-level registration already seals the boundary:
rlangalready setsR_useDynamicSymbols(dll, FALSE)and registers its call routines withR_registerRoutines(), ensuring R itself never attempts dynamic symbol lookup viaGetProcAddress.- The
*-win.deffiles in{stats}and{Matrix}are primarily stylistic/historical Base R conventions to ensure clean PE export headers and prevent third-party C code from linking directly against internal routines instead of usingR_GetCCallable().- Conclusion: Adding
rlang-win.defis harmless and achieves parity with POSIX$(C_VISIBILITY), but there is no compelling technical reason to mandate it acrossr-libpackages or build tooling support inusethis. Leaving it out is completely fine.
R-exts: https://cran.r-project.org/doc/manuals/r-devel/R-exts.html#Creating-shared-objects-1
Follows r-lib/xml2#473 and Rdatatable/data.table#7607.
As noted there, it's not clear this is really needed, but it does follow the practice of other packages like {Matrix}, and signals clear intent.
I got here looking around at
r-librepos with$(C_VISIBILITY)declarations; I would add the equivalent file to all those repos if we think it's not a waste of time :)https://github.com/search?q=org%3Ar-lib+%2F%5B%24%5D%5B%28%5DC_VISIBILITY%2F&type=code