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.

Sign up for GitHub

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

Jump to bottom

tools: add missing uv_setup_argv() calls #35491

Closed
wants to merge 1 commit into from
Closed

tools: add missing uv_setup_argv() calls #35491

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Oct 4, 2020

Refs: #34751

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@addaleax addaleax requested a review from cjihrig October 4, 2020 00:08
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 4, 2020
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 4, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 4, 2020 •
edited by addaleax
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/33374/ (:green_heart:)

@addaleax addaleax added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 6, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 6, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2020

Landed in b5490d0

@github-actions github-actions bot closed this Oct 6, 2020
nodejs-github-bot pushed a commit that referenced this pull request Oct 6, 2020
@addaleax @nodejs-github-bot
tools: add missing uv_setup_argv() calls
b5490d0 
Refs: #34751

PR-URL: #35491
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@danielleadams danielleadams mentioned this pull request Oct 6, 2020
Merged
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
@addaleax @danielleadams
tools: add missing uv_setup_argv() calls
5790c40 
Refs: #34751

PR-URL: #35491
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
BethGriggs added a commit that referenced this pull request Oct 14, 2020
@BethGriggs
Revert "tools: add missing uv_setup_argv() calls"
bd34410 
This reverts commit 70e77f123164ecbdbdd4abc2e4919ce1b193421e.

Refs: #35491

PR-URL: #35641
Reviewed-By: Richard Lau <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
@addaleax @MylesBorins
tools: add missing uv_setup_argv() calls
df152e4 
Refs: #34751

PR-URL: #35491
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@addaleax @MylesBorins
tools: add missing uv_setup_argv() calls
0955267 
Refs: #34751

PR-URL: #35491
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@addaleax @joesepi
tools: add missing uv_setup_argv() calls
aa94935 
Refs: nodejs#34751

PR-URL: nodejs#35491
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request May 30, 2021
@addaleax @targos
tools: add missing uv_setup_argv() calls
fe4ef1c 
Refs: #34751

PR-URL: #35491
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@targos
Copy link
Member

targos commented May 30, 2021

ASAN reports a leak if this is applied to v14.x:

==58694==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 145 byte(s) in 1 object(s) allocated from:
    #0 0x9ed47d in malloc (/home/runner/work/node/node/out/Release/node_mksnapshot+0x9ed47d)
    #1 0xaace3c in uv_setup_args /home/runner/work/node/node/out/../deps/uv/src/unix/proctitle.c:68:14
    #2 0x103d2bb in main (/home/runner/work/node/node/out/Release/node_mksnapshot+0x103d2bb)
    #3 0x7f46755ee0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

SUMMARY: AddressSanitizer: 145 byte(s) leaked in 1 allocation(s).
make[2]: *** [node.target.mk:26: /home/runner/work/node/node/out/Release/obj/gen/node_snapshot.cc] Error 1
make[2]: *** Waiting for unfinished jobs....
rm 2ad06b2c9baade6b27c5c82cdcc7af11c369378d.intermediate 9ac559b3661ef3e3ae7143f36ccc6691e578b31c.intermediate 02a486ff0aa87df511c38b125409357fdc559df2.intermediate 5e4cf980d3dc346b9b9ba01305d21285c5aecbf4.intermediate
make[1]: *** [Makefile:104: node] Error 2
make: *** [Makefile:530: build-ci] Error 2

Do you know what would be missing on the branch to fix it? Otherwise, I guess we can mark dont-land as v14.x seems to work fine without it...

@addaleax addaleax deleted the tools-uv-setup-args branch May 30, 2021 21:40
@addaleax
Copy link
Member Author

@targos I think this would be fine to omit this PR if it doesn’t turn out to be necessary for other work, yes. At the same time, adding uv_library_shutdown() to the main() functions here should be enough to resolve that leak, and I’m not really understanding why it only shows up on v14.x.

Trott pushed a commit to Trott/io.js that referenced this pull request Jun 1, 2021
@addaleax @Trott
tools: add missing uv_setup_argv() calls
22f9ea6 
Refs: nodejs#34751

PR-URL: nodejs#35491
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crypt096 crypt096 crypt096 approved these changes

@richardlau richardlau richardlau approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.