RFR: JDK-8238380: java.base/unix/native/libjava/childproc.c "multiple definition" link errors with GCC10

[Roger Riggs]

+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 <thomas.stuefe at gmail.com>
> Sent: Wednesday, February 5, 2020 11:16 PM
> To: Patrick Zhang OS <patrick at os.amperecomputing.com>
> Cc: core-libs-dev <core-libs-dev at 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 <patrick at os.amperecomputing.com<mailto:patrick at 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.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
> Sent: Wednesday, February 5, 2020 2:30 PM
> To: Patrick Zhang OS <patrick at os.amperecomputing.com<mailto:patrick at os.amperecomputing.com>>
> Cc: core-libs-dev <core-libs-dev at openjdk.java.net<mailto:core-libs-dev at 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 <patrick at os.amperecomputing.com<mailto:patrick at 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 <martinrb at google.com<mailto:martinrb at google.com><mailto:martinrb at google.com<mailto:martinrb at google.com>>>
> Sent: Monday, December 16, 2019 10:44 AM
> To: Patrick Zhang OS <patrick at os.amperecomputing.com<mailto:patrick at os.amperecomputing.com><mailto:patrick at os.amperecomputing.com<mailto:patrick at os.amperecomputing.com>>>; net-dev <net-dev at openjdk.java.net<mailto:net-dev at openjdk.java.net><mailto:net-dev at openjdk.java.net<mailto:net-dev at openjdk.java.net>>>; OpenJDK <security-dev at openjdk.java.net<mailto:security-dev at openjdk.java.net><mailto:security-dev at openjdk.java.net<mailto:security-dev at openjdk.java.net>>>
> Cc: core-libs-dev <core-libs-dev at openjdk.java.net<mailto:core-libs-dev at openjdk.java.net><mailto:core-libs-dev at openjdk.java.net<mailto:core-libs-dev at 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 <patrick at os.amperecomputing.com<mailto:patrick at os.amperecomputing.com><mailto:patrick at os.amperecomputing.com<mailto:patrick at os.amperecomputing.com>>> 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