Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Fri, 8 Mar 2024 10:10:23 GMT, Magnus Ihse Bursie wrote: >> I think what matters for this test test most is which platforms we build >> `jspawnhelper` for, and those platforms indeed are: >> >> >> ifeq ($(call isTargetOs, macosx aix linux), true) >> $(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \ >> >> >> So I'd say we just add `(os.family == "mac")` here. I would find it a bit >> weird to ask for `os.family != "windows"`, which would implicitly rely on >> exhaustiveness of current os family types. > > Hm, that is not ideal code in the makefile. `jspawnhelper` is called from > `src/java.base/unix/classes/java/lang/ProcessImpl.java`, so it is relied upon > for all Unix implementation. Granted, this is currently the same as the list > "macosx aix linux", but it would be better to express the same logic in the > makefile as in the code. JDK-8327675 was just integrated, which replaces the build logic for jspawnhelper to check for "unix" instead of enumerating all our unixes. I suggest the same pattern should be used here. - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1518152090
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Fri, 8 Mar 2024 09:19:43 GMT, Aleksey Shipilev wrote: >> Yes indeed, it is used for all Unix OSes (that is, everything but Windows). > > I think what matters for this test test most is which platforms we build > `jspawnhelper` for, and those platforms indeed are: > > > ifeq ($(call isTargetOs, macosx aix linux), true) > $(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \ > > > So I'd say we just add `(os.family == "mac")` here. I would find it a bit > weird to ask for `os.family != "windows"`, which would implicitly rely on > exhaustiveness of current os family types. Hm, that is not ideal code in the makefile. `jspawnhelper` is called from `src/java.base/unix/classes/java/lang/ProcessImpl.java`, so it is relied upon for all Unix implementation. Granted, this is currently the same as the list "macosx aix linux", but it would be better to express the same logic in the makefile as in the code. - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1517516188
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Fri, 8 Mar 2024 07:35:27 GMT, Magnus Ihse Bursie wrote: >> test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 29: >> >>> 27: * @test >>> 28: * @bug 8325567 >>> 29: * @requires (os.family == "linux") | (os.family == "aix") >> >> Unless I'm mistaken, jspawn helper is used on Mac as well. > > Yes indeed, it is used for all Unix OSes (that is, everything but Windows). I think what matters for this test test most is which platforms we build `jspawnhelper` for, and those platforms indeed are: ifeq ($(call isTargetOs, macosx aix linux), true) $(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \ So I'd say we just add `(os.family == "mac")` here. I would find it a bit weird to ask for `os.family != "windows"`, which would implicitly rely on exhaustiveness of current os family types. - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1517455696
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 20:04:13 GMT, Vladimir Petko wrote: > I wonder if it would make sense to add a test with a correct argument format? I would say that is what current jspawnhelper tests already test, and it is also tested implicitly with `Process` tests. Let's keep this test for testing that warning messages are printed on most common cases of misuse -- that is why `JspawnhelperWarnings` is a good name. - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1517458419
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 19:47:55 GMT, Roger Riggs wrote: >> Elif Aslan has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add args[0] back > > test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 29: > >> 27: * @test >> 28: * @bug 8325567 >> 29: * @requires (os.family == "linux") | (os.family == "aix") > > Unless I'm mistaken, jspawn helper is used on Mac as well. Yes indeed, it is used for all Unix OSes (that is, everything but Windows). - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1517332878
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 19:44:11 GMT, Roger Riggs wrote:
>> Elif Aslan has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add args[0] back
>
> test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 56:
>
>> 54: public static void main(String[] args) throws Exception {
>> 55: for (int nArgs = 0; nArgs < 10; nArgs++) {
>> 56: tryWithNArgs(nArgs);
>
> Running with more than 3 arguments is unnecessary. Yes, its quick but just
> burns cpu.
>
> When running with 2 arguments, the failure mode is not due to the number of
> arguments but is because argument 1 is formatted incorrectly; should be
> `"%d:%d:%d"`. Though I supposed this falls into the "incorrect use category".
I wonder if it would make sense to add a test with a correct argument format?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516754742
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 17:13:12 GMT, Elif Aslan wrote:
>> This change is intended to address the segmentation fault issue that occurs
>> when jspawnhelper is called without arguments,.
>> There is a new test added to verify the behavior in such cases.
>>
>> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test
>> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java`
>>
>>
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java
>> 1 1 0 0
>> ==
>> TEST SUCCESS
>
> Elif Aslan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add args[0] back
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 29:
> 27: * @test
> 28: * @bug 8325567
> 29: * @requires (os.family == "linux") | (os.family == "aix")
Unless I'm mistaken, jspawn helper is used on Mac as well.
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 36:
> 34: import java.nio.file.Paths;
> 35: import java.util.Arrays;
> 36: import java.util.concurrent.TimeUnit;
Unused import.
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 56:
> 54: public static void main(String[] args) throws Exception {
> 55: for (int nArgs = 0; nArgs < 10; nArgs++) {
> 56: tryWithNArgs(nArgs);
Running with more than 3 arguments is unnecessary. Yes, its quick but just
burns cpu.
When running with 2 arguments, the failure mode is not due to the number of
arguments but is because argument 1 is formatted incorrectly; should be
`"%d:%d:%d"`. Though I supposed this falls into the "incorrect use category".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516736692
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516738195
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516733166
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 17:13:12 GMT, Elif Aslan wrote: >> This change is intended to address the segmentation fault issue that occurs >> when jspawnhelper is called without arguments,. >> There is a new test added to verify the behavior in such cases. >> >> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test >> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` >> >> >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java >> 1 1 0 0 >> == >> TEST SUCCESS > > Elif Aslan has updated the pull request incrementally with one additional > commit since the last revision: > > Add args[0] back Marked as reviewed by vpetko (Author). - PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1923293793
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 17:13:12 GMT, Elif Aslan wrote: >> This change is intended to address the segmentation fault issue that occurs >> when jspawnhelper is called without arguments,. >> There is a new test added to verify the behavior in such cases. >> >> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test >> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` >> >> >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java >> 1 1 0 0 >> == >> TEST SUCCESS > > Elif Aslan has updated the pull request incrementally with one additional > commit since the last revision: > > Add args[0] back I am good with this version. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1923081130
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
> This change is intended to address the segmentation fault issue that occurs > when jspawnhelper is called without arguments,. > There is a new test added to verify the behavior in such cases. > > `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test > TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` > > > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java > 1 1 0 0 > == > TEST SUCCESS Elif Aslan has updated the pull request incrementally with one additional commit since the last revision: Add args[0] back - Changes: - all: https://git.openjdk.org/jdk/pull/18112/files - new: https://git.openjdk.org/jdk/pull/18112/files/fb9cb777..c82fb77b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18112=06 - incr: https://webrevs.openjdk.org/?repo=jdk=18112=05-06 Stats: 3 lines in 1 file changed: 1 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18112.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18112/head:pull/18112 PR: https://git.openjdk.org/jdk/pull/18112
