Enable OMR_WARNINGS_AS_ERRORS in the JIT on Power for non-xlC builds #18614

dylanjtuttle · 2023-12-13T15:33:14Z

"Warnings as errors" is enabled on a component-by-component basis throughout OpenJ9 and OMR. For the JIT, warnings as errors is enabled on every platform in OMR and on x86, Z, and Aarch64 in OpenJ9.

This PR enables warnings as errors on Power in OpenJ9 by turning the OMR_WARNINGS_AS_ERRORS flag on if OMR_ARCH_POWER is true. This will ensure all builds on Power compile code with the -Werror flag, which will halt compilation if a warning is reported.

keithc-ca · 2024-05-15T14:27:33Z

runtime/compiler/CMakeLists.txt

@@ -328,7 +328,7 @@ omr_stringify(J9_CXXFLAGS_STR ${J9_SHAREDFLAGS} ${J9_CXXFLAGS})
 set(CMAKE_CXX_FLAGS "${J9_CXXFLAGS_STR}")
 set(CMAKE_C_FLAGS "${J9_CFLAGS_STR}")

-if(OMR_ARCH_X86 OR OMR_ARCH_S390 OR OMR_ARCH_AARCH64)
+if(OMR_ARCH_X86 OR OMR_ARCH_S390 OR OMR_ARCH_AARCH64 OR (OMR_ARCH_POWER AND (__clang__ OR __GNUC__ OR __GNUG__)))


Suggest this should use OMR_TOOLCONFIG instead of __clang__, __GNUC__, or __GNUG__.

I see, perhaps OMR_ARCH_POWER AND NOT OMR_TOOLCONFIG STREQUAL "xlc"?

Yes, if we want to enable warnings for all compilers except xlc, that would be the way to say it.

How many warnings would we have to address to omit the tool-chain part of that expression (i.e. how close are we to fixing the warnings issued by xlc)?

I'll let you know once I find out!

It looks like we have 16 unique types of warnings issued by xlc:

(W) "offsetof" cannot be applied to _. It is not a POD (plain old data) type.

(W) The subscript _ is out of range. The valid range is _.

(W) WARNING in _: Infinite loop. Program may not stop.

(W) WARNING: subprogram _ could not be inlined into _.

(W) Maximum number of common component diagnostics, _ has been exceeded.

(W) Macro name _ has been redefined.

(W) Cannot pass an argument of non-POD class type _ through ellipsis.

(W) An expression of type _ cannot be converted to type _.

(W) The characters "/*" are detected in a comment.

(W) A return value of type _ is expected.

(W) File is empty.

(W) The source file is empty.

(W) _ might be used before it is set.

(W) Compilation unit is empty.

(W) Function argument assignment between types _ and _ is not allowed.

(W) Operation between types _ and _ is not allowed.

Some of those should be easy to fix:

the three flavours of "file/unit is empty" can be avoided by changing the cmake configuration to avoid compiling such files

the "maximum diagnostics" should fix itself once we fix the others

/* in a comment - trivial

The others should be treated as errors, to me they signal broken code (not just warnings).

keithc-ca · 2024-05-15T14:28:09Z

Have you tested with gcc 13 (which is the default version on Ubuntu 24.04)?

dylanjtuttle · 2024-05-15T14:32:27Z

@keithc-ca I don't believe I did, but once I've cleaned up any warnings that appeared while I was gone I'll make sure to do that.

dylanjtuttle · 2024-05-24T19:24:07Z

@keithc-ca I can confirm that no warnings arise from the JIT when building JDK 11 on plinux Ubuntu 20.04 with gcc 13. I actually had to disable warnings-as-errors to complete the build because there were a handful of warnings outside of the JIT that crashed it. I find that a little odd, because I ran plenty of warnings-as-errors builds last year that had non-JIT warnings and had no issues.

To be fair, the only warning that I know crashed the build for certain was the first one, because after that I configured with --disable-warnings-as-errors, --disable-warnings-as-errors-openj9, and --disable-warnings-as-errors-omr, so the others just appeared as regular warnings. I have a feeling that this warning crashes because it arises from openj9/runtime, whereas most (or potentially all) of the non-JIT warnings I saw last year came from places like java.base. Nevertheless, I'm still confused that turning on warnings as errors in runtime/compiler/CMakeLists.txt seems to potentially have an effect on code outside of runtime/compiler.

The warnings in question:
1.

/root/openj9-openjdk-jdk11/openj9/runtime/rastrace/trcmain.c: In function 'initializeTrace':
/root/openj9-openjdk-jdk11/openj9/runtime/rastrace/trcmain.c:1045:14: warning: storing the address of local variable 'tempThr' in '*thr' [-Wdangling-pointer=]
 1045 |         *thr = &tempThr;
      |         ~~~~~^~~~~~~~~~
/root/openj9-openjdk-jdk11/openj9/runtime/rastrace/trcmain.c:1033:23: note: 'tempThr' declared here
 1033 |         UtThreadData  tempThr;
      |                       ^~~~~~~
/root/openj9-openjdk-jdk11/openj9/runtime/rastrace/trcmain.c:1026:32: note: 'thr' declared here
 1026 | initializeTrace(UtThreadData **thr, void **gbl,
      |                 ~~~~~~~~~~~~~~~^~~

/root/openj9-openjdk-jdk11/openj9/runtime/shared_common/CacheMap.cpp: In member function 'const bool SH_CacheMap::matchAotMethod(MethodSpecTable*, IDATA, J9UTF8*, J9UTF8*, J9UTF8*)':
/root/openj9-openjdk-jdk11/openj9/runtime/shared_common/CacheMap.cpp:6330:33: warning: the address of 'J9UTF8::data' will never be NULL [-Waddress]
 6330 |                 } else if (NULL == J9UTF8_DATA(romMethodSig)) {
In file included from /root/openj9-openjdk-jdk11/openj9/runtime/oti/j9generated.h:69,
                 from /root/openj9-openjdk-jdk11/openj9/runtime/oti/j9.h:79,
                 from /root/openj9-openjdk-jdk11/openj9/runtime/shared_common/CacheMapStats.hpp:28,
                 from /root/openj9-openjdk-jdk11/openj9/runtime/shared_common/CacheMap.hpp:27,
                 from /root/openj9-openjdk-jdk11/openj9/runtime/shared_common/CacheMap.cpp:22:
/root/openj9-openjdk-jdk11/openj9/runtime/oti/j9nonbuilder.h:3520:13: note: 'J9UTF8::data' declared here
 3520 |         U_8 data[];
      |             ^~~~

/root/openj9-openjdk-jdk11/openj9/runtime/shared_common/CacheMap.cpp: In member function 'IDATA SH_CacheMap::startupLowerLayerForStats(J9VMThread*, const char*, UDATA, SH_OSCache*, J9Pool**)':
/root/openj9-openjdk-jdk11/openj9/runtime/shared_common/CacheMap.cpp:5991:65: warning: dangling pointer 'cacheName' to 'cacheNameBuf' may be used [-Wdangling-pointer=]
 5991 |                         rc = ccToUse->startupNonTopLayerForStats(currentThread, ctrlDirName, cacheName, cacheType, layer, _runtimeFlags, 0);
      |                              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/root/openj9-openjdk-jdk11/openj9/runtime/shared_common/CacheMap.cpp:5974:30: note: 'cacheNameBuf' declared here
 5974 |                         char cacheNameBuf[USER_SPECIFIED_CACHE_NAME_MAXLEN];
      |                              ^~~~~~~~~~~~

/root/openj9-openjdk-jdk11/openj9/runtime/shared_common/ScopeManagerImpl.cpp: In static member function 'static UDATA SH_ScopeManagerImpl::scHashEqualFn(void*, void*, void*)':
/root/openj9-openjdk-jdk11/openj9/runtime/shared_common/ScopeManagerImpl.cpp:99:34: warning: the address of 'J9UTF8::data' will never be NULL [-Waddress]
   99 |         if (J9UTF8_DATA(utf8left)==NULL || J9UTF8_DATA(utf8right)==NULL) {
In file included from /root/openj9-openjdk-jdk11/openj9/runtime/oti/j9generated.h:69,
                 from /root/openj9-openjdk-jdk11/openj9/runtime/oti/j9.h:79,
                 from /root/openj9-openjdk-jdk11/openj9/runtime/oti/shcdatatypes.h:26,
                 from /root/openj9-openjdk-jdk11/openj9/runtime/shared_common/sharedconsts.h:26,
                 from /root/openj9-openjdk-jdk11/openj9/runtime/shared_common/Manager.hpp:32,
                 from /root/openj9-openjdk-jdk11/openj9/runtime/shared_common/ScopeManager.hpp:32,
                 from /root/openj9-openjdk-jdk11/openj9/runtime/shared_common/ScopeManagerImpl.hpp:32,
                 from /root/openj9-openjdk-jdk11/openj9/runtime/shared_common/ScopeManagerImpl.cpp:28:
/root/openj9-openjdk-jdk11/openj9/runtime/oti/j9nonbuilder.h:3520:13: note: 'J9UTF8::data' declared here
 3520 |         U_8 data[];
      |             ^~~~
/root/openj9-openjdk-jdk11/openj9/runtime/shared_common/ScopeManagerImpl.cpp:99:66: warning: the address of 'J9UTF8::data' will never be NULL [-Waddress]
   99 |         if (J9UTF8_DATA(utf8left)==NULL || J9UTF8_DATA(utf8right)==NULL) {
/root/openj9-openjdk-jdk11/openj9/runtime/oti/j9nonbuilder.h:3520:13: note: 'J9UTF8::data' declared here
 3520 |         U_8 data[];
      |             ^~~~

/root/openj9-openjdk-jdk11/openj9/runtime/libffi/closures.c: In function 'dlmmap_locked':
/root/openj9-openjdk-jdk11/openj9/runtime/libffi/closures.c:495:7: warning: ignoring return value of 'ftruncate' declared with attribute 'warn_unused_result' [-Wunused-result]
  495 |       ftruncate (execfd, offset);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~
/root/openj9-openjdk-jdk11/openj9/runtime/libffi/closures.c:507:7: warning: ignoring return value of 'ftruncate' declared with attribute 'warn_unused_result' [-Wunused-result]
  507 |       ftruncate (execfd, offset);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~

/root/openj9-openjdk-jdk11/openj9/runtime/tests/shared/ByteDataTest.cpp: In function 'IDATA testByteDataManager(J9JavaVM*)':
/root/openj9-openjdk-jdk11/openj9/runtime/tests/shared/ByteDataTest.cpp:112:55: warning: 'cachePointers.CachePointers::testCache2' may be used uninitialized [-Wmaybe-uninitialized]
  112 |         if (cachePointers.testCache1 && cachePointers.testCache2) {
      |                                         ~~~~~~~~~~~~~~^~~~~~~~~~
/root/openj9-openjdk-jdk11/openj9/runtime/tests/shared/ByteDataTest.cpp:98:30: note: 'cachePointers' declared here
   98 |         struct CachePointers cachePointers;
      |                              ^~~~~~~~~~~~~

/root/openj9-openjdk-jdk11/src/java.base/unix/native/libjli/java_md_common.c: In function 'Resolve':
/root/openj9-openjdk-jdk11/src/java.base/unix/native/libjli/java_md_common.c:131:43: warning: '%s' directive output may be truncated writing up to 4095 bytes into a region of size between 2 and 4097 [-Wformat-truncation=]
  131 |     JLI_Snprintf(name, sizeof(name), "%s%c%s", indir, FILE_SEPARATOR, cmd);
      |                                           ^~
In file included from /usr/include/stdio.h:867,
                 from /root/openj9-openjdk-jdk11/src/java.base/share/native/libjli/java.h:29,
                 from /root/openj9-openjdk-jdk11/src/java.base/unix/native/libjli/java_md_common.c:25:
In function 'snprintf',
    inlined from 'Resolve' at /root/openj9-openjdk-jdk11/src/java.base/unix/native/libjli/java_md_common.c:131:5:
/usr/include/powerpc64le-linux-gnu/bits/stdio2.h:67:10: note: '__builtin___snprintf_chk' output between 2 and 8192 bytes into a destination of size 4098
   67 |   return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   68 |                                    __bos (__s), __fmt, __va_arg_pack ());
      |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

keithc-ca · 2024-05-27T15:11:04Z

I saw (1) in my xlinux build with gcc 13; that one will require some more thought.

Warnings (2) and (4) should be easy to address: simply remove the dead code.

Warning (3) may just require moving the declaration to a larger scope so the target survives as long as the pointer.

I tried to address warning (5) some time ago, but I remember having trouble telling the compiler to ignore the return value. That is code we inherit from https://github.com/libffi/libffi so any improvements should be shared there as well.

Warning (6) should be straight-forward: add initialization visible to the compiler.

Issue (7) is in openjdk code; I'm not sure jdk11 has been kept current with newer compliers. Even the head stream, the minimum gcc version is 10.

dylanjtuttle · 2024-06-03T15:38:54Z

@keithc-ca Do you know how it's possible for a change in the JIT's CMakeLists.txt to be affecting non-JIT code? That seems dangerous. From an offline conversation with @hzongaro, neither of us seem to know why this would be happening, so as far as we know, any change to the JIT's CMake configuration could theoretically break any other part of the code for seemingly no reason (kind of like it did in this case with OMR_WARNINGS_AS_ERRORS).

With a better understanding of why this CMake dependency between the JIT and other components exists, perhaps we could figure out how to fix it, or at least be better informed about when it's safe to change the JIT's CMakeLists.txt and when it isn't.

keithc-ca · 2024-06-04T14:43:59Z

Do you know how it's possible for a change in the JIT's CMakeLists.txt to be affecting non-JIT code?

The only thing that I can think of is that OMR_WARNINGS_AS_ERRORS is a CACHE variable, which effectively lives in a scope outside the project, and so affects the whole project. In the end, we want all warnings to be fatal, so we should just fix any we find.

dylanjtuttle · 2024-06-06T13:35:36Z

Understood. I'll start working on those warnings!

dylanjtuttle · 2024-06-11T15:21:04Z

I hadn't actually tested this at the time, but it turns out that even the warning from the openjdk code is treated as an error. Would that be a PR to https://github.com/ibmruntimes/openj9-openjdk-jdk11? If so, I suppose I should check all of our JDK versions...

keithc-ca · 2024-06-11T15:31:03Z

I hadn't actually tested this at the time, but it turns out that even the warning from the openjdk code is treated as an error. Would that be a PR to https://github.com/ibmruntimes/openj9-openjdk-jdk11? If so, I suppose I should check all of our JDK versions...

We have three independent configuration options:

--disable-warnings-as-errors for openjdk code
--disable-warnings-as-errors-openj9 for OpenJ9 code
--disable-warnings-as-errors-omr for OMR code

jdk11 may not be ready for newer compilers, in which case we should just use the first option. OpenJ9 code and OMR code should not have warnings even without using the latter two options.

dylanjtuttle · 2024-06-11T15:46:30Z

I see, I'll just focus on the OpenJ9 and OMR warnings then.

dylanjtuttle · 2024-09-24T13:47:52Z

@keithc-ca @pshipton @zl-wang Now that the last warning has been disabled, could I get a review on this when one or more of you has a chance?

runtime/compiler/CMakeLists.txt

dylanjtuttle · 2024-09-27T17:20:45Z

@keithc-ca How would I run a build of IBM Java 8 on AIX? That is the last build we have that still uses the xlC frontend and I would like to confirm that this change doesn't break that build.

keithc-ca

Please squash.

Signed-off-by: Dylan Tuttle <[email protected]>

dylanjtuttle · 2024-09-27T18:15:45Z

Done!

keithc-ca · 2024-09-27T18:59:41Z

Jenkins test sanity plinux jdk23

keithc-ca · 2024-09-27T22:09:46Z

The test failure appears to be #17474.

hzongaro mentioned this pull request May 15, 2024

gcc10 warnings in the JIT #14859

Open

keithc-ca reviewed May 15, 2024

View reviewed changes

dylanjtuttle mentioned this pull request Jun 11, 2024

Fix warnings in shared_common, libffi, tests/shared #19674

Merged

dylanjtuttle mentioned this pull request Sep 19, 2024

Disable -Winvalid-offsetof in J9Options, kca_offsets_generator, J9ValueProfiler #18313

Merged

dylanjtuttle force-pushed the enableWAEPower branch from ef351a1 to 0d8a5ec Compare September 19, 2024 20:11

dylanjtuttle changed the title ~~Enable OMR_WARNINGS_AS_ERRORS in the JIT on Power for clang and GCC builds~~ Enable OMR_WARNINGS_AS_ERRORS in the JIT on Power Sep 19, 2024

dylanjtuttle force-pushed the enableWAEPower branch from 0d8a5ec to 1a24e10 Compare September 19, 2024 20:42

dylanjtuttle changed the title ~~Enable OMR_WARNINGS_AS_ERRORS in the JIT on Power~~ Enable OMR_WARNINGS_AS_ERRORS in the JIT on Power for non-xlC builds Sep 19, 2024

dylanjtuttle marked this pull request as ready for review September 20, 2024 17:14

keithc-ca reviewed Sep 24, 2024

View reviewed changes

runtime/compiler/CMakeLists.txt Outdated Show resolved Hide resolved

zl-wang requested changes Sep 24, 2024

View reviewed changes

runtime/compiler/CMakeLists.txt Show resolved Hide resolved

zl-wang approved these changes Sep 27, 2024

View reviewed changes

keithc-ca requested changes Sep 27, 2024

View reviewed changes

Enable OMR_WARNINGS_AS_ERRORS on Power for non-xlC builds

5fb4b0a

Signed-off-by: Dylan Tuttle <[email protected]>

dylanjtuttle force-pushed the enableWAEPower branch from 422674a to 5fb4b0a Compare September 27, 2024 18:14

keithc-ca approved these changes Sep 27, 2024

View reviewed changes

keithc-ca merged commit 0027b81 into eclipse-openj9:master Sep 27, 2024
4 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable OMR_WARNINGS_AS_ERRORS in the JIT on Power for non-xlC builds #18614

Enable OMR_WARNINGS_AS_ERRORS in the JIT on Power for non-xlC builds #18614

dylanjtuttle commented Dec 13, 2023 •

edited

Loading

keithc-ca May 15, 2024

dylanjtuttle May 15, 2024 •

edited

Loading

keithc-ca May 15, 2024

dylanjtuttle May 15, 2024

dylanjtuttle May 15, 2024

keithc-ca May 15, 2024

keithc-ca commented May 15, 2024

dylanjtuttle commented May 15, 2024

dylanjtuttle commented May 24, 2024

keithc-ca commented May 27, 2024

dylanjtuttle commented Jun 3, 2024

keithc-ca commented Jun 4, 2024

dylanjtuttle commented Jun 6, 2024

dylanjtuttle commented Jun 11, 2024

keithc-ca commented Jun 11, 2024

dylanjtuttle commented Jun 11, 2024

dylanjtuttle commented Sep 24, 2024

dylanjtuttle commented Sep 27, 2024

keithc-ca left a comment

dylanjtuttle commented Sep 27, 2024

keithc-ca commented Sep 27, 2024

keithc-ca commented Sep 27, 2024

Enable OMR_WARNINGS_AS_ERRORS in the JIT on Power for non-xlC builds #18614

Enable OMR_WARNINGS_AS_ERRORS in the JIT on Power for non-xlC builds #18614

Conversation

dylanjtuttle commented Dec 13, 2023 • edited Loading

keithc-ca May 15, 2024

Choose a reason for hiding this comment

dylanjtuttle May 15, 2024 • edited Loading

Choose a reason for hiding this comment

keithc-ca May 15, 2024

Choose a reason for hiding this comment

dylanjtuttle May 15, 2024

Choose a reason for hiding this comment

dylanjtuttle May 15, 2024

Choose a reason for hiding this comment

keithc-ca May 15, 2024

Choose a reason for hiding this comment

keithc-ca commented May 15, 2024

dylanjtuttle commented May 15, 2024

dylanjtuttle commented May 24, 2024

keithc-ca commented May 27, 2024

dylanjtuttle commented Jun 3, 2024

keithc-ca commented Jun 4, 2024

dylanjtuttle commented Jun 6, 2024

dylanjtuttle commented Jun 11, 2024

keithc-ca commented Jun 11, 2024

dylanjtuttle commented Jun 11, 2024

dylanjtuttle commented Sep 24, 2024

dylanjtuttle commented Sep 27, 2024

keithc-ca left a comment

Choose a reason for hiding this comment

dylanjtuttle commented Sep 27, 2024

keithc-ca commented Sep 27, 2024

keithc-ca commented Sep 27, 2024

dylanjtuttle commented Dec 13, 2023 •

edited

Loading

dylanjtuttle May 15, 2024 •

edited

Loading