FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: add missing uv_setup_argv() calls by addaleax · Pull Request #35491 · nodejs/node · GitHub

/ node Public

tools: add missing uv_setup_argv() calls - #35491

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:tools-uv-setup-args
ConversationCommits (1)ChecksFiles changed
Closed

tools: add missing uv_setup_argv() calls#35491
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:tools-uv-setup-args

Conversation

addaleax commented Oct 4, 2020

Copy link
Copy Markdown
Member

Refs: #34751

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

addaleax requested a review from cjihrig October 4, 2020 00:08

nodejs-github-bot commented Oct 4, 2020

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Oct 4, 2020
addaleax added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 4, 2020
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 commented Oct 4, 2020 •
edited by addaleax
Loading

Copy link
Copy Markdown
Collaborator

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

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

github-actions Bot commented Oct 6, 2020

Copy link
Copy Markdown
Contributor

Landed in b5490d0

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

PR-URL: #35491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
danielleadams mentioned this pull request Oct 6, 2020
Merged
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
tools: add missing uv_setup_argv() calls
5790c40 
Refs: #34751

PR-URL: #35491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BethGriggs added a commit that referenced this pull request Oct 14, 2020
Revert "tools: add missing uv_setup_argv() calls"
bd34410 
This reverts commit 70e77f123164ecbdbdd4abc2e4919ce1b193421e.

Refs: #35491

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

PR-URL: #35491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins mentioned this pull request Nov 3, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
tools: add missing uv_setup_argv() calls
0955267 
Refs: #34751

PR-URL: #35491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
tools: add missing uv_setup_argv() calls
aa94935 
Refs: nodejs#34751

PR-URL: nodejs#35491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request May 30, 2021
tools: add missing uv_setup_argv() calls
fe4ef1c 
Refs: #34751

PR-URL: #35491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

targos commented May 30, 2021

Copy link
Copy Markdown
Member

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 deleted the tools-uv-setup-args branch May 30, 2021 21:40

addaleax commented May 30, 2021

Copy link
Copy Markdown
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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
Trott Trott approved these changes
cjihrig cjihrig approved these changes
tniessen tniessen approved these changes
joyeecheung joyeecheung approved these changes
richardlau richardlau approved these changes
+1 more reviewer
crypt096 crypt096 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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.

Back | FazBrowse Home | New Git URL