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

[Hohensee, Paul]

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
>