RE: RFR: JDK-8238386: (sctp) jdk.sctp/unix/native/libsctp/SctpNet.c "multiple definition" link errors with GCC10
Ping.. Regards Patrick -Original Message- From: net-dev On Behalf Of Patrick Zhang OS Sent: Wednesday, February 5, 2020 8:19 PM To: Chris Hegarty ; net-dev Subject: RE: RFR: JDK-8238386: (sctp) jdk.sctp/unix/native/libsctp/SctpNet.c "multiple definition" link errors with GCC10 Thanks Chris I extracted the SCTP code change from the original patch, for net-dev review only. Could you please sponsor this? (I do not have push permission), thanks in advance. JBS: https://bugs.openjdk.java.net/browse/JDK-8238386 Webrev: http://cr.openjdk.java.net/~qpzhang/8238386/webrev.01/ Test: ran jtreg tier1, jdk built with GCC4.8.5, GCC9, and GCC10, no regression found Regards Patrick -Original Message- From: Chris Hegarty Sent: Tuesday, December 17, 2019 7:06 PM To: Patrick Zhang OS Cc: net-dev ; OpenJDK ; core-libs-dev Subject: Re: RFR: JDK-8235903: GCC default -fno-common exposes "multiple definition" link errors The changes to the SCTP code seem ok. -Chris. > On 17 Dec 2019, at 03:00, Patrick Zhang OS > wrote: > > Thanks Martin. > > Hi net-dev, and/or security-dev Reviewers, > > Please help review and sponsor this patch if acceptable. > It does not tend to bring any functionality changes, instead to propose an > early fix, for the build (linking) errors with further toolchain (GCC 10). > > JBS: https://bugs.openjdk.java.net/browse/JDK-8235903 > Webrev: http://cr.openjdk.java.net/~qpzhang/8235903/webrev.01/ > > Regards > Patrick > > From: Martin Buchholz > Sent: Monday, December 16, 2019 10:44 AM > To: Patrick Zhang OS ; net-dev > ; OpenJDK > Cc: core-libs-dev > 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 > mailto:patr...@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-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
RFR[15] 8238677: java/net/httpclient/ssltest/CertificateTest.java should not specify TLS version
Hi, java/net/httpclient/ssltest/CertificateTest.java shouldn't use a specific TLS version. And it would be better not to use binary key store files. Since DSA is not supported by TLSv1.3, this fix also updates the certificates to use RSA. Webrev: http://cr.openjdk.java.net/~jjiang/8238677/webrev.00/ Issue: https://bugs.openjdk.java.net/browse/JDK-8238677 Best regards, John Jiang
Re: RFR[15] 8238677: java/net/httpclient/ssltest/CertificateTest.java should not specify TLS version
Hi John, Looks good to me. Thanks for taking care of this! I'm glad to see the binary files go away :-) Would it be possible to include a comment in Cert.java that contains the command you used to generate the certificates? That will be a great help to future maintainers if the certificates ever needs to be re-generated (e.g. to update the expiry date etc...) best regards, -- daniel Disclaimer: I am not an expert in ssl/security On 07/02/2020 10:51, sha.ji...@oracle.com wrote: Hi, java/net/httpclient/ssltest/CertificateTest.java shouldn't use a specific TLS version. And it would be better not to use binary key store files. Since DSA is not supported by TLSv1.3, this fix also updates the certificates to use RSA. Webrev: http://cr.openjdk.java.net/~jjiang/8238677/webrev.00/ Issue: https://bugs.openjdk.java.net/browse/JDK-8238677 Best regards, John Jiang
Re: RFR: JDK-8238386: (sctp) jdk.sctp/unix/native/libsctp/SctpNet.c "multiple definition" link errors with GCC10
Patrick,
> On 7 Feb 2020, at 08:44, Patrick Zhang OS
> wrote:
>
> Ping..
Pong ;-)
> Regards
> Patrick
>
> -Original Message-
> From: net-dev On Behalf Of Patrick Zhang OS
> Sent: Wednesday, February 5, 2020 8:19 PM
> To: Chris Hegarty ; net-dev
>
> Subject: RE: RFR: JDK-8238386: (sctp) jdk.sctp/unix/native/libsctp/SctpNet.c
> "multiple definition" link errors with GCC10
>
> Thanks Chris
>
> I extracted the SCTP code change from the original patch, for net-dev review
> only. Could you please sponsor this? (I do not have push permission), thanks
> in advance.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8238386
> Webrev: http://cr.openjdk.java.net/~qpzhang/8238386/webrev.01/
I was able to duplicate the "multiple definition of ..." warnings/errors
on my Ubuntu Linux with gcc 8.3 by explicitly specifying -fno-common,
temporarily locally patching Lib-jdk.sctp.gmk [*].
Looking at this again, maybe we can avoid the creation of a new Sctp.c
file to "house" these few definitions. How about we just move them to
SctpNet.?
$ hg diff src/jdk.sctp
diff --git a/src/jdk.sctp/unix/native/libsctp/Sctp.h
b/src/jdk.sctp/unix/native/libsctp/Sctp.h
--- a/src/jdk.sctp/unix/native/libsctp/Sctp.h
+++ b/src/jdk.sctp/unix/native/libsctp/Sctp.h
@@ -322,13 +322,6 @@
#endif /* __linux__ */
-sctp_getladdrs_func* nio_sctp_getladdrs;
-sctp_freeladdrs_func* nio_sctp_freeladdrs;
-sctp_getpaddrs_func* nio_sctp_getpaddrs;
-sctp_freepaddrs_func* nio_sctp_freepaddrs;
-sctp_bindx_func* nio_sctp_bindx;
-sctp_peeloff_func* nio_sctp_peeloff;
-
jboolean loadSocketExtensionFuncs(JNIEnv* env);
#endif /* !SUN_NIO_CH_SCTP_H */
diff --git a/src/jdk.sctp/unix/native/libsctp/SctpNet.c
b/src/jdk.sctp/unix/native/libsctp/SctpNet.c
--- a/src/jdk.sctp/unix/native/libsctp/SctpNet.c
+++ b/src/jdk.sctp/unix/native/libsctp/SctpNet.c
@@ -43,6 +43,13 @@
static const char* nativeSctpLib = "libsctp.so.1";
static jboolean funcsLoaded = JNI_FALSE;
+sctp_getladdrs_func* nio_sctp_getladdrs;
+sctp_freeladdrs_func* nio_sctp_freeladdrs;
+sctp_getpaddrs_func* nio_sctp_getpaddrs;
+sctp_freepaddrs_func* nio_sctp_freepaddrs;
+sctp_bindx_func* nio_sctp_bindx;
+sctp_peeloff_func* nio_sctp_peeloff;
+
JNIEXPORT jint JNICALL DEF_JNI_OnLoad
(JavaVM *vm, void *reserved) {
return JNI_VERSION_1_2;
-Chris.
[*] diff --git a/make/lib/Lib-jdk.sctp.gmk b/make/lib/Lib-jdk.sctp.gmk
--- a/make/lib/Lib-jdk.sctp.gmk
+++ b/make/lib/Lib-jdk.sctp.gmk
@@ -33,7 +33,7 @@
$(eval $(call SetupJdkLibrary, BUILD_LIBSCTP, \
NAME := sctp, \
OPTIMIZATION := LOW, \
-CFLAGS := $(CFLAGS_JDKLIB), \
+CFLAGS := -fno-common $(CFLAGS_JDKLIB), \
DISABLED_WARNINGS_gcc := undef, \
DISABLED_WARNINGS_clang := undef, \
EXTRA_HEADER_DIRS := \
Re: RFR[15] 8238677: java/net/httpclient/ssltest/CertificateTest.java should not specify TLS version
Hi Daniel, On 2020/2/7 19:29, Daniel Fuchs wrote: Hi John, Looks good to me. Thanks for taking care of this! I'm glad to see the binary files go away :-) Thanks for your review! Would it be possible to include a comment in Cert.java that contains the command you used to generate the certificates? That will be a great help to future maintainers if the certificates ever needs to be re-generated (e.g. to update the expiry date etc...) I'll do that. Best regards, John Jiang best regards, -- daniel Disclaimer: I am not an expert in ssl/security On 07/02/2020 10:51, sha.ji...@oracle.com wrote: Hi, java/net/httpclient/ssltest/CertificateTest.java shouldn't use a specific TLS version. And it would be better not to use binary key store files. Since DSA is not supported by TLSv1.3, this fix also updates the certificates to use RSA. Webrev: http://cr.openjdk.java.net/~jjiang/8238677/webrev.00/ Issue: https://bugs.openjdk.java.net/browse/JDK-8238677 Best regards, John Jiang
RE: RFR: JDK-8238386: (sctp) jdk.sctp/unix/native/libsctp/SctpNet.c "multiple definition" link errors with GCC10
Agree, I was wondering where to place them, SctpNet.c or SctpChannelImpl.c.
Here is the update: http://cr.openjdk.java.net/~qpzhang/8238386/webrev.02/
(smoke tested the build only)
And, thanks for your double-check with the modified Lib-jdk.sctp.gmk. :)
Regards
Patrick
-Original Message-
From: Chris Hegarty
Sent: Friday, February 7, 2020 8:10 PM
To: Patrick Zhang OS
Cc: net-dev
Subject: Re: RFR: JDK-8238386: (sctp) jdk.sctp/unix/native/libsctp/SctpNet.c
"multiple definition" link errors with GCC10
Patrick,
> On 7 Feb 2020, at 08:44, Patrick Zhang OS
> wrote:
>
> Ping..
Pong ;-)
> Regards
> Patrick
>
> -Original Message-
> From: net-dev On Behalf Of Patrick
> Zhang OS
> Sent: Wednesday, February 5, 2020 8:19 PM
> To: Chris Hegarty ; net-dev
>
> Subject: RE: RFR: JDK-8238386: (sctp)
> jdk.sctp/unix/native/libsctp/SctpNet.c "multiple definition" link
> errors with GCC10
>
> Thanks Chris
>
> I extracted the SCTP code change from the original patch, for net-dev review
> only. Could you please sponsor this? (I do not have push permission), thanks
> in advance.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8238386
> Webrev: http://cr.openjdk.java.net/~qpzhang/8238386/webrev.01/
I was able to duplicate the "multiple definition of ..." warnings/errors on my
Ubuntu Linux with gcc 8.3 by explicitly specifying -fno-common, temporarily
locally patching Lib-jdk.sctp.gmk [*].
Looking at this again, maybe we can avoid the creation of a new Sctp.c file to
"house" these few definitions. How about we just move them to SctpNet.?
$ hg diff src/jdk.sctp
diff --git a/src/jdk.sctp/unix/native/libsctp/Sctp.h
b/src/jdk.sctp/unix/native/libsctp/Sctp.h
--- a/src/jdk.sctp/unix/native/libsctp/Sctp.h
+++ b/src/jdk.sctp/unix/native/libsctp/Sctp.h
@@ -322,13 +322,6 @@
#endif /* __linux__ */
-sctp_getladdrs_func* nio_sctp_getladdrs;
-sctp_freeladdrs_func* nio_sctp_freeladdrs;
-sctp_getpaddrs_func* nio_sctp_getpaddrs;
-sctp_freepaddrs_func* nio_sctp_freepaddrs;
-sctp_bindx_func* nio_sctp_bindx;
-sctp_peeloff_func* nio_sctp_peeloff;
-
jboolean loadSocketExtensionFuncs(JNIEnv* env);
#endif /* !SUN_NIO_CH_SCTP_H */
diff --git a/src/jdk.sctp/unix/native/libsctp/SctpNet.c
b/src/jdk.sctp/unix/native/libsctp/SctpNet.c
--- a/src/jdk.sctp/unix/native/libsctp/SctpNet.c
+++ b/src/jdk.sctp/unix/native/libsctp/SctpNet.c
@@ -43,6 +43,13 @@
static const char* nativeSctpLib = "libsctp.so.1"; static jboolean
funcsLoaded = JNI_FALSE;
+sctp_getladdrs_func* nio_sctp_getladdrs;
+sctp_freeladdrs_func* nio_sctp_freeladdrs;
+sctp_getpaddrs_func* nio_sctp_getpaddrs;
+sctp_freepaddrs_func* nio_sctp_freepaddrs;
+sctp_bindx_func* nio_sctp_bindx;
+sctp_peeloff_func* nio_sctp_peeloff;
+
JNIEXPORT jint JNICALL DEF_JNI_OnLoad
(JavaVM *vm, void *reserved) {
return JNI_VERSION_1_2;
-Chris.
[*] diff --git a/make/lib/Lib-jdk.sctp.gmk b/make/lib/Lib-jdk.sctp.gmk
--- a/make/lib/Lib-jdk.sctp.gmk
+++ b/make/lib/Lib-jdk.sctp.gmk
@@ -33,7 +33,7 @@
$(eval $(call SetupJdkLibrary, BUILD_LIBSCTP, \
NAME := sctp, \
OPTIMIZATION := LOW, \
-CFLAGS := $(CFLAGS_JDKLIB), \
+CFLAGS := -fno-common $(CFLAGS_JDKLIB), \
DISABLED_WARNINGS_gcc := undef, \
DISABLED_WARNINGS_clang := undef, \
EXTRA_HEADER_DIRS := \
Re: RFR: JDK-8238386: (sctp) jdk.sctp/unix/native/libsctp/SctpNet.c "multiple definition" link errors with GCC10
> On 7 Feb 2020, at 14:08, Patrick Zhang OS > wrote: > > Agree, I was wondering where to place them, SctpNet.c or SctpChannelImpl.c. > Here is the update: http://cr.openjdk.java.net/~qpzhang/8238386/webrev.02/ > (smoke tested the build only) > And, thanks for your double-check with the modified Lib-jdk.sctp.gmk. :) I will run this through our internal build and test system, then report back here. -Chris.
Re: RFR[15] 8238677: java/net/httpclient/ssltest/CertificateTest.java should not specify TLS version
Hi Daniel, Would it be possible to include a comment in Cert.java that contains the command you used to generate the certificates? That will be a great help to future maintainers if the certificates ever needs to be re-generated (e.g. to update the expiry date etc...) I'll do that. Please review this updated webrev: http://cr.openjdk.java.net/~jjiang/8238677/webrev.01/ The script, exactly gen-certs.sh, can be used to generate the certs. Best regards, John Jiang Best regards, John Jiang best regards, -- daniel Disclaimer: I am not an expert in ssl/security On 07/02/2020 10:51, sha.ji...@oracle.com wrote: Hi, java/net/httpclient/ssltest/CertificateTest.java shouldn't use a specific TLS version. And it would be better not to use binary key store files. Since DSA is not supported by TLSv1.3, this fix also updates the certificates to use RSA. Webrev: http://cr.openjdk.java.net/~jjiang/8238677/webrev.00/ Issue: https://bugs.openjdk.java.net/browse/JDK-8238677 Best regards, John Jiang
