Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link libuv & zlib with --whole-archive #17444

Closed
addaleax opened this issue Dec 4, 2017 · 10 comments
Closed

Link libuv & zlib with --whole-archive #17444

addaleax opened this issue Dec 4, 2017 · 10 comments
Labels
build Issues and PRs related to build files or the CI. libuv Issues and PRs related to the libuv dependency or the uv binding. zlib Issues and PRs related to the zlib subsystem.

Comments

@addaleax
Copy link
Member

addaleax commented Dec 4, 2017

  • Version: all
  • Platform: probably not Windows, maybe only Linux
  • Subsystem: deps/libuv

Currently, we link V8 and openssl with -Wl,--whole-archive into the node binary, but we should probably do the same for libuv and maybe zlib since we want to expose their entire functionality to userland addons.

/cc @refack @bnoordhuis

@addaleax addaleax added build Issues and PRs related to build files or the CI. libuv Issues and PRs related to the libuv dependency or the uv binding. zlib Issues and PRs related to the zlib subsystem. labels Dec 4, 2017
@refack
Copy link
Contributor

refack commented Dec 4, 2017

This is definatly an interesting suggestion. Some points that come to mind:

  1. For libuv this is probably an easier decision...
  2. Does it affect the binary size?
  3. node implicitly taking responsibility of those libraries' ABI
  4. Is this just a convenience or does it protect addons from dll-hell (linking to two incompatible zlib files for example)?
  5. How does this fit with the N-API roadmap.

Bottom line, I think I'm +1 on this.

@refack
Copy link
Contributor

refack commented Dec 4, 2017

P.S. I just saw that the full headers files for both zlib and libuv are in the headers tarball, so this is actually a bugfix.

P.P.S. Relevant also for Windows https://docs.microsoft.com/en-us/cpp/build/reference/wholearchive-include-all-library-object-files where it is probably even more usefull since there is no real apt-get install zlib-devel

@addaleax
Copy link
Member Author

addaleax commented Dec 5, 2017

Does it affect the binary size?

Probably not all that much – these libs are tiny compared to e.g. V8, and they don’t follow a one-function-per-file pattern.

How does this fit with the N-API roadmap.

Fwiw I raised this issue in response to not seeing the symbols from libuv/libuv#1657 included in the Node binary, which in turn was made with the intention of having libuv be ABI-stable indefinitely.

P.S. I just saw that the full headers files for both zlib and libuv are in the headers tarball, so this is actually a bugfix.

Yeah, exactly. :)

@richardlau
Copy link
Member

  • Platform: probably not Windows, maybe only Linux

FWIW On AIX we already export these symbols (and more) via tools/create_expfile.sh.

@mhdawson
Copy link
Member

I'm +1 as my assumption is today we just get a semi-random set (those used within Node.js) which does not make sense given we include the full headers.

@addaleax
Copy link
Member Author

Btw, I opened this as an issue because I really don’t know enough about GYP to have this figured out myself at this point. I’m convinced it should happen, though.

The obvious approach could be copying what we do for openssl, but that seems like it’s done in a kind of hacky way to begin with.

@mhdawson
Copy link
Member

mhdawson commented Dec 13, 2017

@yhwang, I know you have been working on the static/shared lib which has required a number of changes to the gyp files and even overcoming issues with symbols not being included. Any chance you could take a look at this issue ?

@yhwang
Copy link
Member

yhwang commented Dec 14, 2017

Sure this is what I learned/know so for:
The obj files that contain the export functions used by node_zlib.cc and uv.cc are included in our node executable. So I guess most of the functions are in. And yes, --whole-archive, force_load could help us to include all functions. In Windows/AIX, we need .exp file to explicitly list the functions. And @richardlau is right, we have that for AIX for now. @digitalinfinity suggested that we should generate .exp for Windows and I agree. We also need to improve the way we generate the .exp.

Adding --whole-archive and force_load is easy in gyp. However, improving the symbol generator for AIX and Windows may need a little more work. Right now, it finds all static archive files and list all symbols inside these static libs. We should explicitly specify what components we want to export and only dump their symbols.

I can work on the exp generator after I finish the static/shared lib refine task. Then enable the libuv and zlib would be easy (I guess :-p).

[edit] Forgot to mention that we can also compile node into shared and static lib with current node.gyp.

  • for static lib, we only have our node_xxx objs. no other lib
  • for shared lib, it would be the same as executable. So those --whole-archive, force_load and .exp stuffs also need to apply to shared lib as well.

@yhwang
Copy link
Member

yhwang commented Dec 17, 2017

Update more findings. Actually, for zlib, all zlib are in the node executable now. the mkssldef target in the node.gyp generates the def file for openssl and zlib. therefore the thing we need to do is add libuv into it.

@bnoordhuis
Copy link
Member

We have mkssldef because openssl and zlib don't export functions inline but libuv does (the UV_EXTERN macro.) Nothing more than --whole-archive should be needed.

yhwang added a commit to yhwang/node that referenced this issue Feb 1, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.

Fixes: nodejs#17444

Signed-off-by: Yihong Wang <[email protected]>
yhwang added a commit to yhwang/node that referenced this issue Feb 22, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.

Signed-off-by: Yihong Wang <[email protected]>

Fixes: nodejs#17444
PR-URL: nodejs#18383
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this issue Feb 26, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.

Signed-off-by: Yihong Wang <[email protected]>

Fixes: #17444
PR-URL: #18383
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this issue Feb 26, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.

Signed-off-by: Yihong Wang <[email protected]>

Fixes: #17444
PR-URL: #18383
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
richardlau added a commit that referenced this issue Mar 2, 2018
libuv and zlib symbols are also purposefully re-exported by Node.js for
use in Addons.

Refs: #17444

PR-URL: #19013
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this issue Mar 5, 2018
libuv and zlib symbols are also purposefully re-exported by Node.js for
use in Addons.

Refs: nodejs#17444

PR-URL: nodejs#19013
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.

Signed-off-by: Yihong Wang <[email protected]>

Fixes: nodejs#17444
PR-URL: nodejs#18383
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
libuv and zlib symbols are also purposefully re-exported by Node.js for
use in Addons.

Refs: nodejs#17444

PR-URL: nodejs#19013
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit that referenced this issue Jun 29, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.

Signed-off-by: Yihong Wang <[email protected]>

Fixes: #17444
PR-URL: #18383
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this issue Aug 16, 2018
Add libuv and zlib into node executable and shared lib. Also fix an
issue that openssl is not fully included in node executable for macOS.

Signed-off-by: Yihong Wang <[email protected]>

Fixes: #17444
PR-URL: #18383
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit to jasnell/node that referenced this issue Aug 17, 2018
libuv and zlib symbols are also purposefully re-exported by Node.js for
use in Addons.

Refs: nodejs#17444

PR-URL: nodejs#19013
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Sep 6, 2018
libuv and zlib symbols are also purposefully re-exported by Node.js for
use in Addons.

Refs: #17444

Backport-PR-URL: #22380
PR-URL: #19013
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
libuv and zlib symbols are also purposefully re-exported by Node.js for
use in Addons.

Refs: nodejs/node#17444
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. libuv Issues and PRs related to the libuv dependency or the uv binding. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants