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

[Volker Simonis]

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
> >
>