[8u] RFR: 8037825: Fix warnings and enable "warnings as errors" in serviceability native libraries

[Hohensee, Paul]

Thanks, Volker. Tagged

-----Original Message-----
From: Volker Simonis <volker.simonis at gmail.com>
Date: Wednesday, May 19, 2021 at 3:34 AM
To: "Hohensee, Paul" <hohensee at amazon.com>
Cc: "jdk8u-dev at openjdk.java.net" <jdk8u-dev at openjdk.java.net>
Subject: RE: [8u] RFR: 8037825: Fix warnings and enable "warnings as errors" in serviceability native libraries

Thanks for the explanation and the adjustments Paul.

The downport looks good to me now.

Best regards,
Volker

On Tue, May 18, 2021 at 11:50 PM Hohensee, Paul <hohensee at amazon.com> wrote:
>
> Thanks, Volker, for the thorough review.
>
> New 8u jdk webrev: https://cr.openjdk.java.net/~phh/8037825/webrev.8u.jdk.01/
>
> Yes, I intentionally only backported the jdk part of the patch. Enabling CFLAGS_WARNINGS_AS_ERRORS does cause compilation errors and imo is overkill for the purpose of getting 8u to build using Xcode 12. If/when we backport all of 8182299, we can backport the root part of this patch.
>
> I added back the change to InvocationAdapter.c, though it's at a different place in the context of the exported changeset. It looked to me like it was already applied in that context so I didn't look for another. There must have been a previous mixup. Good catch.
>
> I changed CFLAGS_CFLAGS_WARNINGS_ARE_ERRORS to CFLAGS_WARNINGS_ARE_ERRORS because it's an obvious mistake. If there's a conflict with a subsequent backport, that backport can be adjusted.
>
> -----Original Message-----
> From: Volker Simonis <volker.simonis at gmail.com>
> Date: Monday, May 17, 2021 at 12:44 PM
> To: "Hohensee, Paul" <hohensee at amazon.com>
> Cc: "jdk8u-dev at openjdk.java.net" <jdk8u-dev at openjdk.java.net>
> Subject: RE: [8u] RFR: 8037825: Fix warnings and enable "warnings as errors" in serviceability native libraries
>
> Hi Paul,
>
> have you intentionally only downported the "jdk/" part of this change
> and omitted the "top-level" part
> (http://hg.openjdk.java.net/jdk9/jdk9/rev/1cf2abab835f)?
>
> If yes, that should probably be mentioned in the commit message and
> the downport request. Otherwise, you'll also have to add the
> "top-level" part. But that might be a little tricky/risky because it
> could potentially introduce new build failures because of the new
> "CFLAGS_WARNINGS_ARE_ERRORS" settings in conjunction with the jdk8u
> compiler toolchains.
>
> Besides that I've spotted two other small issues:
>
>  - I can't see that the change in InvocationAdapter.c was already applied:
> http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/tip/src/share/instrument/InvocationAdapter.c#l376
>
> Have you maybe confused it with this occurrence?
> http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/tip/src/share/instrument/InvocationAdapter.c#l204
>
> - The following hunk looks pretty much like an error (notice the use
> of "CFLAGS_CFLAGS_WARNINGS_ARE_ERRORS" instead of
> "CFLAGS_WARNINGS_ARE_ERRORS" ), but it was already an error in the
> initial patch so I'm not sure if it's worth fixing in the downport:
>
> make/lib/ServiceabilityLibraries.gmk
> -----------------------------------------------
> @@ -81,11 +81,11 @@
>      OUTPUT_DIR := $(INSTALL_LIBRARIES_HERE), \
>      SRC := $(JDK_TOPDIR)/src/share/transport/socket \
>          $(JDK_TOPDIR)/src/$(OPENJDK_TARGET_OS_API_DIR)/transport/socket, \
>      LANG := C, \
>      OPTIMIZATION := LOW, \
> -    CFLAGS := $(CFLAGS_JDKLIB) -DUSE_MMAP \
> +    CFLAGS := $(CFLAGS_JDKLIB) $(CFLAGS_CFLAGS_WARNINGS_ARE_ERRORS)
> -DUSE_MMAP \
>
> Thank you and best regards,
> Volker
>
> On Thu, Apr 29, 2021 at 11:58 PM Hohensee, Paul <hohensee at amazon.com> wrote:
> >
> > Please review this backport to help 8u build using Xcode 12.
> >
> > Original issue: https://bugs.openjdk.java.net/browse/JDK-8037825
> > Original commit: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/f5a18100873c
> > 8u webrev: https://cr.openjdk.java.net/~phh/8037825/webrev.8u.jdk.00/
> >
> > The patch applied cleanly, except that the change to InvocationAdapter.c was already in place, so was left out of the backport. There are no semantic changes.
> >
> > Thanks,
> > Paul
> >
>