As far as another reviewer is needed - looks good to me, too 😊 /Christoph
> -----Original Message----- > From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> On Behalf > Of Thomas Stüfe > Sent: Donnerstag, 6. Februar 2020 07:49 > To: Patrick Zhang OS <patr...@os.amperecomputing.com> > Cc: core-libs-dev <core-libs-dev@openjdk.java.net> > Subject: Re: RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c > "multiple definition" link errors with GCC10 > > I can sponsor this for you. > > .. Thomas > > On Thu, Feb 6, 2020, 04:35 Patrick Zhang OS > <patr...@os.amperecomputing.com> > 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 <thomas.stu...@gmail.com> > > *Sent:* Wednesday, February 5, 2020 11:16 PM > > *To:* Patrick Zhang OS <patr...@os.amperecomputing.com> > > *Cc:* core-libs-dev <core-libs-dev@openjdk.java.net> > > *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 < > > patr...@os.amperecomputing.com> 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 <thomas.stu...@gmail.com> > > *Sent:* Wednesday, February 5, 2020 2:30 PM > > *To:* Patrick Zhang OS <patr...@os.amperecomputing.com> > > *Cc:* core-libs-dev <core-libs-dev@openjdk.java.net> > > *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 < > > patr...@os.amperecomputing.com> 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 > <marti...@google.com<mailto:marti...@google.com>> > > Sent: Monday, December 16, 2019 10:44 AM > > To: Patrick Zhang OS <patr...@os.amperecomputing.com<mailto: > > patr...@os.amperecomputing.com>>; net-dev <net- > d...@openjdk.java.net > > <mailto:net-...@openjdk.java.net>>; OpenJDK <security- > d...@openjdk.java.net > > <mailto:security-...@openjdk.java.net>> > > Cc: core-libs-dev <core-libs-dev@openjdk.java.net<mailto: > > core-libs-dev@openjdk.java.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 < > > > patr...@os.amperecomputing.com<mailto:patr...@os.amperecomputing.co > m>> > > 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- > common > > > > 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 > > > >
