8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c #18013

jianglizhou · 2024-02-26T20:20:31Z

Please help review this trivial fix for resolving ld: error: duplicate symbol: closeDescriptors when static linking with both libjdwp and libjava, thanks.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c (Bug - P4)

Reviewers

Chris Plummer (@plummercj - Reviewer)
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18013/head:pull/18013
$ git checkout pull/18013

Update a local copy of the PR:
$ git checkout pull/18013
$ git pull https://git.openjdk.org/jdk.git pull/18013/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18013

View PR using the GUI difftool:
$ git pr show -t 18013

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18013.diff

Webrev

Link to Webrev Comment

…ve/libjava/childproc.c and src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c.

bridgekeeper · 2024-02-26T20:21:20Z

👋 Welcome back jiangli! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-02-26T20:21:57Z

@jianglizhou The following labels will be automatically applied to this pull request:

core-libs
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-02-26T20:25:51Z

Webrevs

01: Full - Incremental (3816d315)
00: Full (bb9e2791)

plummercj · 2024-02-26T20:37:45Z

src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c

@@ -62,7 +62,7 @@ static char *skipNonWhitespace(char *p) {
 // input/output/error file descriptors will not be closed
 // by this function. This function returns 0 on failure
 // and 1 on success.
-int
+static int


I think you should also make forkedChildProcess static. It was added at the same time.

plummercj · 2024-02-26T20:40:52Z

src/java.base/unix/native/libjava/childproc.h

@@ -131,7 +131,6 @@ ssize_t writeFully(int fd, const void *buf, size_t count);
 int restartableDup2(int fd_from, int fd_to);
 int closeSafely(int fd);
 int isAsciiDigit(char c);
-int closeDescriptors(void);


It seems that most of the APIs in this file should be static. I don't think you should selectively deal with just one of them because of the conflict. Since this ends up being a more involved change, and is in a different component than the jdwp change, it should probably have a separate PR.

@plummercj thanks for looking into this! Sounds good to make the additional local functions static in these files. Perhaps we can use two different FRs. I'll make the current one to handle the ones in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c.

Filed https://bugs.openjdk.org/browse/JDK-8326714.

…e/unix/native/libjava/childproc.h. Will handle those with JDK-8326714.

plummercj

Looks good.

openjdk · 2024-02-26T23:50:45Z

@jianglizhou This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c

Reviewed-by: cjplummer, sspitsyn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 6 new commits pushed to the master branch:

d22d890: 8325898: ChoiceFormat returns erroneous result when formatting bad pattern
93feda3: 8322962: Upcall stub might go undetected when freezing frames
fc67c2b: 8325530: Vague error message when com.sun.tools.attach.VirtualMachine fails to load agent library
b87d9cf: 8325506: Ensure randomness is only read from provided SecureRandom object
0963a4e: 8326699: Problemlist CertMsgCheck.java
bf13a4e: 8322881: java/nio/file/Files/CopyMoveVariations.java fails with AccessDeniedException due to permissions of files in /tmp

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

jianglizhou · 2024-02-26T23:56:48Z

Looks good.

Thanks for the quick review, @plummercj.

jianglizhou · 2024-02-27T01:33:52Z

Thanks for the review, @sspitsyn.

jianglizhou · 2024-02-27T01:34:02Z

/integrate

openjdk · 2024-02-27T01:34:20Z

Going to push as commit 0901ded.
Since your change was applied there have been 6 commits pushed to the master branch:

d22d890: 8325898: ChoiceFormat returns erroneous result when formatting bad pattern
93feda3: 8322962: Upcall stub might go undetected when freezing frames
fc67c2b: 8325530: Vague error message when com.sun.tools.attach.VirtualMachine fails to load agent library
b87d9cf: 8325506: Ensure randomness is only read from provided SecureRandom object
0963a4e: 8326699: Problemlist CertMsgCheck.java
bf13a4e: 8322881: java/nio/file/Files/CopyMoveVariations.java fails with AccessDeniedException due to permissions of files in /tmp

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-27T01:34:27Z

@jianglizhou Pushed as commit 0901ded.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Make closeDescriptors() as static function in src/java.base/unix/nati…

bb9e279

…ve/libjava/childproc.c and src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c.

openjdk bot added the rfr label Feb 26, 2024

openjdk bot added serviceability core-libs labels Feb 26, 2024

plummercj reviewed Feb 26, 2024

View reviewed changes

jianglizhou added 2 commits February 26, 2024 14:41

Revert src/java.base/unix/native/libjava/childproc.c and src/java.bas…

c2bd090

…e/unix/native/libjava/childproc.h. Will handle those with JDK-8326714.

Address plummercj's comment and make forkedChildProcess static.

3816d31

jianglizhou changed the title ~~8326433: Make libjdwp and libjava closeDescriptors() as static function~~ 8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c Feb 26, 2024

plummercj approved these changes Feb 26, 2024

View reviewed changes

openjdk bot added the ready label Feb 26, 2024

sspitsyn approved these changes Feb 27, 2024

View reviewed changes

openjdk bot added the integrated label Feb 27, 2024

openjdk bot closed this Feb 27, 2024

openjdk bot removed ready rfr labels Feb 27, 2024

jianglizhou mentioned this pull request Feb 27, 2024

8326714: Make file-local functions static in src/java.base/unix/native/libjava/childproc.c #18019

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c #18013

8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c #18013

jianglizhou commented Feb 26, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 26, 2024

openjdk bot commented Feb 26, 2024

mlbridge bot commented Feb 26, 2024 •

edited

Loading

plummercj Feb 26, 2024

jianglizhou Feb 26, 2024

plummercj Feb 26, 2024

jianglizhou Feb 26, 2024

jianglizhou Feb 26, 2024

plummercj left a comment

openjdk bot commented Feb 26, 2024 •

edited

Loading

jianglizhou commented Feb 26, 2024

jianglizhou commented Feb 27, 2024

jianglizhou commented Feb 27, 2024

openjdk bot commented Feb 27, 2024

openjdk bot commented Feb 27, 2024

8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c #18013

8326433: Make file-local functions static in src/jdk.jdwp.agent/unix/native/libjdwp/exec_md.c #18013

Conversation

jianglizhou commented Feb 26, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 26, 2024

openjdk bot commented Feb 26, 2024

mlbridge bot commented Feb 26, 2024 • edited Loading

Webrevs

plummercj Feb 26, 2024

Choose a reason for hiding this comment

jianglizhou Feb 26, 2024

Choose a reason for hiding this comment

plummercj Feb 26, 2024

Choose a reason for hiding this comment

jianglizhou Feb 26, 2024

Choose a reason for hiding this comment

jianglizhou Feb 26, 2024

Choose a reason for hiding this comment

plummercj left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 26, 2024 • edited Loading

jianglizhou commented Feb 26, 2024

jianglizhou commented Feb 27, 2024

jianglizhou commented Feb 27, 2024

openjdk bot commented Feb 27, 2024

openjdk bot commented Feb 27, 2024

jianglizhou commented Feb 26, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 26, 2024 •

edited

Loading

openjdk bot commented Feb 26, 2024 •

edited

Loading