RFR: JDK-8308288: Fix xlc17 clang warnings in shared code

Kim Barrett kbarrett at openjdk.org
Thu May 25 15:23:58 UTC 2023


On Thu, 25 May 2023 09:14:14 GMT, JoKern65 <duke at openjdk.org> wrote:

> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk on AIX , we run into various "warnings as errors".
> Some of those are in shared codebase and could be addressed by small adjustments.
> A lot of those changes are in hotspot, some might be somewhere else in the OpenJDK C/C++ code.

Sorry, but I can't review the warning suppressions. Without more information
about exactly what warnings are being suppressed and where, I have no way to
evaluate whether suppression is an appropriate response. It could be that some
of the warnings are correctly pointing out (possibly serious) flaws that
really need to be fixed.  Continuing to hide them doesn't seem like a winning
strategy to me.

I also suggest avoiding omnibus changes like this when possible (which it is
here). Just because it's all about removing warnings from a new toolchain
doesn't make it a cohesive change. That makes it harder to review and to
manage the review, because it is larger and affects a wide range of areas, so
may need many reviewers. And the whole thing might get stalled by discussion
of one piece.

make/modules/java.desktop/lib/Awt2dLibraries.gmk line 706:

> 704:   # temporarily disable PNG_POWERPC_VSX_OPT which would be set, because
> 705:   # if defined(__PPC64__) && defined(__ALTIVEC__) && defined(__VSX__)
> 706:   # is true, the then needed libpng function .png_init_filter_functions_vsx

s/then//

src/hotspot/share/utilities/globalDefinitions_xlc.hpp line 47:

> 45:   #undef malloc
> 46:   extern void *malloc(size_t) asm("vec_malloc");
> 47: #endif

Wow!  And I don't mean that in a good way.  I'm not questioning whether this is correct, just commenting
on how crazy it seems that such a thing might be needed.

test/jdk/java/io/File/libGetXSpace.c line 128:

> 126: #else
> 127:     struct statfs buf;
> 128:     int result = statfs((char*)chars, &buf);

Is this working around a bug in IBM's declaration?

Also, pre-existing, the cast seems really suspicious.  The type of `chars` is `jchar*`, which is a
sequence of 16bit characters.  Does this actually work?  If so, how?

-------------

PR Review: https://git.openjdk.org/jdk/pull/14146#pullrequestreview-1444057072
PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1205622153
PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1205655676
PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1205673657


More information about the hotspot-dev mailing list