Thank you all. Hi Thomas, please help push this: http://cr.openjdk.java.net/~qpzhang/8238380/webrev.03, I updated the reviewer list accordingly. Thanks.
Regards Patrick -----Original Message----- From: core-libs-dev <[email protected]> On Behalf Of Roger Riggs Sent: Thursday, February 6, 2020 10:52 PM To: [email protected] Subject: Re: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link errors with GCC10 +1, One is enough, but additional eyes are good too. On 2/5/20 10:35 PM, Patrick Zhang OS wrote: > Does this require one more “yes” from any other reviewer? > > And may I ask for any committer’s help to sponsor and push it (once > approved)? Thanks in advance. > > Regards > Patrick > > From: Thomas Stüfe <[email protected]> > Sent: Wednesday, February 5, 2020 11:16 PM > To: Patrick Zhang OS <[email protected]> > Cc: core-libs-dev <[email protected]> > Subject: Re: RFR: JDK-8238380: > java.base/unix/native/libjava/childproc.c "multiple definition" link > errors with GCC10 > > All still good. > > ...Thomas > > On Wed, Feb 5, 2020 at 3:42 PM Patrick Zhang OS > <[email protected]<mailto:[email protected]>> wrote: > Thanks for your comments, Thomas. > > I have no accurate knowledge regarding why parentPathv was initially written > as a global var instead of a member in ChildStuff struct initialized in > jspawnhelper.c. However the suggested change might not be very > straight-forward as there are other references such as: > Java_java_lang_ProcessImpl_init(), spawnChild() in libjava\ProcessImpl_md.c, > etc. And this goes beyond my original intention to fix the build errors > purely, and try best to avoid any side effect. So I would leave the further > improvement to others, and keep it as-is. Thanks. > > Updated the change a bit (the header comments): > http://cr.openjdk.java.net/~qpzhang/8238380/webrev.02/ > > Regards > Patrick > > From: Thomas Stüfe > <[email protected]<mailto:[email protected]>> > Sent: Wednesday, February 5, 2020 2:30 PM > To: Patrick Zhang OS > <[email protected]<mailto:[email protected]> > > > Cc: core-libs-dev > <[email protected]<mailto:[email protected]> > > > Subject: Re: RFR: JDK-8238380: > java.base/unix/native/libjava/childproc.c "multiple definition" link > errors with GCC10 > > Hi Patrick, > > You patch looks alright. > > But I wonder whether a cleaner solution would not be to add a parentPathv > pointer to the ChildStuff structure and pass it down to JDK_execvpe that way, > like we do for all other input arguments of that function. If it is an input > argument, seems strange to pass it as global variable. I leave that for > others to decide though, your patch certainly works. > > Cheers, Thomas > > On Tue, Feb 4, 2020 at 3:39 PM Patrick Zhang OS > <[email protected]<mailto:[email protected]>> wrote: > Hi > > I split the original patch into parts for corresponding function review, here > is the very simple change to java.base/unix/native/libjava/childproc.c and .h. > Could core-libs-dev group help review and sponsor this? thanks in advance. > > JBS: https://bugs.openjdk.java.net/browse/JDK-8238380 (a sub-task of > JDK-8235903) > Webrev: http://cr.openjdk.java.net/~qpzhang/8238380/webrev.01/ > Test: ran jtreg tier1, jdk built with GCC4.8.5, GCC9, and GCC10, no > regression found. I don’t have any specific function tests, so this is just a > smoke test. > > Regards > Patrick > > From: Martin Buchholz > <[email protected]<mailto:[email protected]><mailto:martinrb@googl > e.com<mailto:[email protected]>>> > Sent: Monday, December 16, 2019 10:44 AM > To: Patrick Zhang OS > <[email protected]<mailto:[email protected]> > <mailto:[email protected]<mailto:[email protected] > ng.com>>>; net-dev > <[email protected]<mailto:[email protected]><mailto:net- > [email protected]<mailto:[email protected]>>>; OpenJDK > <[email protected]<mailto:[email protected]><m > ailto:[email protected]<mailto:[email protected] > et>>> > Cc: core-libs-dev > <[email protected]<mailto:[email protected]> > <mailto:[email protected]<mailto:[email protected] > va.net>>> > Subject: Re: RFR: JDK-8235903: GCC default -fno-common exposes > "multiple definition" link errors > > forwarded to other teams for review. > > On Fri, Dec 13, 2019 at 3:14 AM Patrick Zhang OS > <[email protected]<mailto:[email protected]><mailto:[email protected]<mailto:[email protected]>>> > wrote: > Hi > > Please review this patch, if it should be reviewed by any group other than > core-libs, please help forward it. Thanks. > > JBS: https://bugs.openjdk.java.net/browse/JDK-8235903 > Webrev: http://cr.openjdk.java.net/~qpzhang/8235903/webrev.01/ > > A recent GCC patch (supposed to be in GCC 10) exposes a couple of "multiple > definition" link errors when building the jdk tip. > > [PATCH] PR85678: Change default to -fno-common > https://gcc.gnu.org/ml/gcc-patches/2019-10/msg01847.html > > For example, the error message looks like: > * For target support_native_java.base_libjava_BUILD_LIBJAVA_link: > build/support/native/java.base/libjava/childproc.o:(.bss+0x0): multiple > definition of `parentPathv' > build/support/native/java.base/libjava/ProcessImpl_md.o:(.bss+0x0): > first defined here > collect2: error: ld returned 1 exit status > > This was not an issue because the original default -fcommon allowed "global > variables defined without an initializer" be handled as COMMON symbols, so it > would not warn the problem like "same variable is accidentally defined in > more than one compilation unit". > > About -fcommon vs -fno-cmmon: > https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fno-com > mon > > Moving forward, building jdk with latest versions of GCC will trigger this > error. Specifying "--with-extra-cflags='-fcommon'" can make it work, but it > just got things hidden again. > > In addition, -fcommon's behavior "is inconsistent with C++, and on many > targets implies a speed and code size penalty on global variable references. > It is mainly useful to enable legacy code to link without errors." > > Last, in case that other jdk developers would revisit this problem once > again, I suggest fixing the error explicitly instead of using "-fcommon" > > Regards > Patrick
