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

dylanjtuttle · 2023-10-19T20:17:05Z

In order to allow OMR_WARNINGS_AS_ERRORS to be enabled across the JIT, disable -Winvalid-offsetof while work is done to refactor relevant classes to be standard-layout type so offsetof can be used without crashing the build on AIX.

hzongaro · 2023-11-01T13:57:20Z

Julian @zl-wang, may I ask you to review this change?

zl-wang · 2023-11-01T17:53:14Z

it sounds strange to me that you are using GCC on AIX as the build compiler (?). or, you actually meant ppc64le?

dylanjtuttle · 2023-11-01T18:10:53Z

That does sound strange now that I think of it, but this is indeed AIX, and I tried both #pragma report(disable, "CCN6281") and #pragma GCC diagnostic ignored "-Winvalid-offsetof" on internal Jenkins AIX builds. As far as I could tell, only the latter was able to remove the warning. Is it possible that one of our AIX machines on Jenkins is using GCC?

Either way, I see now how it would be important to include both of those directives like in omr #6077. I never actually tried running a build with both (because it seemed like we only need to disable the warning on AIX), so I'll give that a try and see what happens.

dylanjtuttle · 2023-11-02T16:03:35Z

After running a build I can confirm that guarding the uses of offsetof like so:

#if defined(LINUX)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Winvalid-offsetof"
#elif defined(OSX)
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Winvalid-offsetof"
#elif defined(AIXPPC) || defined(J9ZOS390)
#pragma report(disable, "CCN6281")
#endif
      fprintf( file, "#define BODYINFO_HOTNESS           (%" OMR_PRIuSIZE ")\n", offsetof(TR_PersistentJittedBodyInfo,_hotness) );
      fprintf( file, "#define PERSISTENTINFO_CHTABLE     (%" OMR_PRIuSIZE ")\n", offsetof(TR::PersistentInfo,_persistentCHTable) );
      fprintf( file, "#define PERSISTENTCLASS_VISITED    (%" OMR_PRIuSIZE ")\n", offsetof(TR_PersistentClassInfo,_visitedStatus) );
#if defined(LINUX)
#pragma GCC diagnostic pop
#elif defined(OSX)
#pragma clang diagnostic pop
#elif defined(AIXPPC) || defined(J9ZOS390)
#pragma report(enable, "CCN6281")
#endif

does not stop the warnings from appearing (see this internal build, all 11 instances of -Winvalid-offsetof still appear).

I'm assuming there's an issue with the pragma report(disable, "CCN6281") directive, then. CCN6281 looks like the right kind of warning code, although that's according to the z/OS documentation because I couldn't find anything helpful in the AIX documentation (I assume the warning code would be the same on both platforms since both are using XLC). On the other hand, the message in that documentation cites the old definition involving non-POD classes:

CCN6281 "offsetof" cannot be applied to "%1$s". It is not a POD (plain old data) type.

while the warning messages I've been getting cite the updated definition involving non-standard-layout classes:

warning: offset of on non-standard-layout type 'TR_PersistentJittedBodyInfo' [-Winvalid-offsetof]

Is it possible that this new warning needs to be referenced or disabled differently?

zl-wang · 2023-11-02T16:48:30Z

it might be too optimistic to assume same CCN# across p and z. i will try to get the xlC/AIX message file ... that allows you to search for CCN# by a particular message. I assumed you are using xlC16 on AIX.

zl-wang · 2023-11-02T17:54:30Z

cppckern.txt
This is the xlC/AIX message file, allowing you to search for the CCN# by messages.

dylanjtuttle · 2023-11-02T18:00:07Z

Thanks! Interestingly, there were two matches with slightly different verbiage, and like you assumed, neither have the same CCN# as the message in the z/OS documentation.

287 "offsetof" must not be applied to "%1$s".  It is not a POD (plain old data) type.
...
1281 "offsetof" cannot be applied to "%1$s".  It is not a POD (plain old data) type.

I'll try disabling these CCN#s instead and see if that works.

zl-wang · 2023-11-02T18:02:33Z

you can certainly find out which CCN# to apply by looking at the generated warning message

dylanjtuttle · 2023-11-02T19:29:23Z

It seems like neither CCN287 nor CCN1281 worked, as another internal build still has the warnings present despite these new guards:

#if defined(LINUX)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Winvalid-offsetof"
#elif defined(OSX)
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Winvalid-offsetof"
#elif defined(J9ZOS390)
#pragma report(disable, "CCN6281")
#elif defined(AIXPPC)
#pragma report(disable, "CCN1281")
#pragma report(disable, "CCN287")
#endif
      fprintf( file, "#define BODYINFO_HOTNESS           (%" OMR_PRIuSIZE ")\n", offsetof(TR_PersistentJittedBodyInfo,_hotness) );
      fprintf( file, "#define PERSISTENTINFO_CHTABLE     (%" OMR_PRIuSIZE ")\n", offsetof(TR::PersistentInfo,_persistentCHTable) );
      fprintf( file, "#define PERSISTENTCLASS_VISITED    (%" OMR_PRIuSIZE ")\n", offsetof(TR_PersistentClassInfo,_visitedStatus) );
#if defined(LINUX)
#pragma GCC diagnostic pop
#elif defined(OSX)
#pragma clang diagnostic pop
#elif defined(J9ZOS390)
#pragma report(enable, "CCN6281")
#elif defined(AIXPPC)
#pragma report(enable, "CCN1281")
#pragma report(enable, "CCN287")
#endif

Is there anything else we can try?

zl-wang · 2023-11-03T17:12:20Z

what exactly is the warning message? does it match with either of the CCNs in file? more fundamentally, are you using xlC16 on AIX?

dylanjtuttle · 2023-11-03T17:58:34Z

We are using xlC16 on AIX:

configure: xlclang++ output: IBM XL C/C++ for AIX, V16.1.0  (5725-C72, 5765-J12)

We get the following warning message:

14:55:37  /home/jenkins/workspace/Build_JDK11_ppc64_aix_Personal/openj9/runtime/compiler/ras/kca_offsets_generator.cpp:220:82: warning: offset of on non-standard-layout type 'TR_PersistentJittedBodyInfo' [-Winvalid-offsetof]
14:55:37        fprintf( file, "#define BODYINFO_HOTNESS           (%" OMR_PRIuSIZE ")\n", offsetof(TR_PersistentJittedBodyInfo,_hotness) );
14:55:37                                                                                   ^                                    ~~~~~~~~
14:55:37  /opt/IBM/xlC/16.1.0/include2/stddef.h:120:24: note: expanded from macro 'offsetof'
14:55:37  #define offsetof(t, d) __builtin_offsetof(t, d)
14:55:37                         ^                     ~

which does not match any of the CCNs in the file. Here are all of the CCNs in the file that mention "offsetof":

262 The first argument to the "offsetof" macro must be a class type.
267 The second operand to the "offsetof" macro is not valid.
287 "offsetof" must not be applied to "%1$s".  It is not a POD (plain old data) type.
1281 "offsetof" cannot be applied to "%1$s".  It is not a POD (plain old data) type.

CCN287 and CCN1281 come close, but they reference "POD" classes, while the warning message we see in the build references "standard layout" classes. Before C++11, offsetof was defined to be incompatible with POD classes, but from C++11 onward that definition was refined to only include standard layout classes [1]. I go into a little bit more detail on that here.

You can actually see the old warning message in this issue from 2019:

"/home/u0020236/workspace/rwyoung/omr/compiler/control/OMROptions.cpp", line 104.77: 1540-1281 (W) "offsetof" cannot be applied to "class Options".  It is not a POD (plain old data) type.

where you can see the message matches CCN1281 from the file, and even has the number 1281 in the prefix to the message. The issue doesn't state what platform this warning is coming from, but we can assume it's AIX from the consistency with the CCN file.

To make things even more confusing, in this even older issue from 2017, we see the same "standard layout type" warning on x86:

[5/128] Building CXX object jitbuilder/CMakeFiles/jitbuilder.dir/__/compiler/env/FEBase.cpp.o
../compiler/env/FEBase.cpp:244:46: warning: offset of on non-standard-layout type 'OMR::FrontEnd::JitConfig' (aka 'JitBuilder::JitConfig') [-Winvalid-offsetof]
    TR::Options::set32BitNumericInJitConfig, offsetof(OMR::FrontEnd::JitConfig, options.codeCacheKB), 0, " %d (KB)"},
                                             ^                                  ~~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/clang/8.1.0/include/stddef.h:120:24: note: expanded from macro 'offsetof'
#define offsetof(t, d) __builtin_offsetof(t, d)
                       ^                     ~
../compiler/env/FEBase.cpp:250:34: warning: offset of on non-standard-layout type 'TR::JitConfig' [-Winvalid-offsetof]
    TR::Options::setVerboseBits, offsetof(TR::JitConfig, options.verboseFlags), (1<<TR_VerboseCompileStart)|(1<<TR_VerboseCompileEnd), "F=1"},
                                 ^                       ~~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/clang/8.1.0/include/stddef.h:120:24: note: expanded from macro 'offsetof'
#define offsetof(t, d) __builtin_offsetof(t, d)

I'm not sure why this build, which is clearly using xlC 16 on AIX, would have GCC-style warnings. It's not just this one either, because if you look at a random nightly AIX build there are over 1000 instances of "[-W" in the build log. But there are also xlC style warnings, as there are 51 instances of "(W)" in that same build log.

zl-wang · 2023-11-03T18:07:28Z

GCC-style warning ... I guessed it is due to using Clang front-end (indicated in your first line).
I also noticed the message is not offsetof, but "offset of". I might alert xlC team to take a look later on.

zl-wang · 2023-11-03T18:30:31Z

could you try these out?

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Winvalid-offsetof"

dylanjtuttle · 2023-11-03T21:13:39Z

That worked! The warnings are gone.

dylanjtuttle · 2023-12-05T18:40:39Z

@zl-wang Should we move forward with the clang directives?

zl-wang · 2023-12-05T20:59:55Z

we knew, starting java15, we used xlC16. But older jdk builds used xlC13 (I believed). xlC13 was not clang based. the same sources were used in the different builds. so, addressing every build is your goal, or only java15-and-later? or, you need to apply version-specific pragma.

pshipton · 2023-12-05T21:07:38Z

aix on jdk8 uses xlc 13, 11+ uses xlc 16.
Also IBM builds use xlc 13 for plinux BE.

dylanjtuttle · 2023-12-05T21:25:01Z

That's good to know. Taking a look at a recent jdk8 AIX nightly build we do see the xlC style warnings:

21:25:44  "/home/jenkins/workspace/Build_JDK8_ppc64_aix_Nightly/omr/compiler/control/OMROptions.cpp", line 420.95: 1540-1281 (W) "offsetof" cannot be applied to "class Options".  It is not a POD (plain old data) type.

We do need to make this change for every build, so we will need to apply the version-specific pragma. How do we do that?

pshipton · 2023-12-05T22:33:59Z

The best way is to test for different compilers using the macros they provide.
i.e. https://www.ibm.com/docs/ru/xl-c-and-cpp-linux/16.1.0?topic=macros-identify-xl-cc-compiler

zl-wang · 2023-12-05T22:39:03Z

do similarly as linux/macOS/AIX. just for xlC, that is.

zl-wang · 2023-12-06T15:18:23Z

runtime/compiler/runtime/J9ValueProfiler.hpp

+#elif defined(OSX) || (defined(AIXPPC) && JAVA_SPEC_VERSION >= 11)
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Winvalid-offsetof"
+#elif defined(AIXPPC) && JAVA_SPEC_VERSION < 11


Linux BE build appeared missing (as mentioned by Peter).

What does BE mean? How is it different from regular plinux builds?

BE means Big Endian. We also have plinux Little Endian builds.
OpenJ9 only builds LE, IBM has both.

it looks not right. Linux BE build used the same (or similar) compiler as AIX java8 (instead of java11+).

It should test the compiler levels and not java levels, as we should be free to change which compilers are used to compile the different versions without having to modify the code.

agreed. you might be able to do that by testing predefined relevant macros existing or not, e.g. xlc and clang etc.

Ok I think I did the control flow logic right this time, I'll run a build and see if the warnings go away

pshipton · 2023-12-07T20:34:01Z

runtime/compiler/control/J9Options.cpp

+#if defined(USE_CLANG)
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Winvalid-offsetof"
+#elif defined(TR_TARGET_POWER)


plinux LE uses gcc

dylanjtuttle · 2023-12-08T17:42:58Z

runtime/compiler/control/J9Options.cpp

@@ -57,7 +57,32 @@
 #include "j9jitnls.h"
 #endif

+#if defined(COMPILER_GCC)


I found these COMPILER_XXX macros in omr/compiler/env/defines.h. Aside from USE_CLANG I couldn't find any other macros that would let us check which compiler we're using in OpenJ9. I was having a hard time figuring out if we've imported these macros across OpenJ9, but I don't think so?

The compilers themselves provide macros. For example https://www.ibm.com/docs/ru/xl-c-and-cpp-linux/16.1.0?topic=macros-identify-xl-cc-compiler

I see, my mistake. I added those along with similar macros for clang and gcc.

dylanjtuttle · 2023-12-12T15:18:20Z

I ran a build to see if the AIX warnings would go away. They're gone on Java 11+ (clang frontend), but they're still present on Java 8:

15:37:19  "/home/jenkins/workspace/Build_JDK8_ppc64_aix_Personal/openj9/runtime/compiler/control/J9Options.cpp", line 978.9: 1540-1281 (W) "offsetof" cannot be applied to "class Options".  It is not a POD (plain old data) type.

The #pragma report(disable, "CCN1281") directive doesn't seem to be working, even though it identifies the correct warning number.

In order to allow OMR_WARNINGS_AS_ERRORS to be enabled across the JIT, disable -Winvalid-offsetof on gcc and clang builds while work is done to refactor relevant classes to be standard-layout type so offsetof can be used Signed-off-by: Dylan Tuttle <jdylantuttle@gmail.com>

dylanjtuttle · 2024-09-19T19:59:39Z

Recently, the version of xlC that is used to compile JDK 8 on AIX was updated to 16.1, which (obviously among other things) replaces the old front end with a Clang-based one. The PR in which the change was made can be found here.

The reason this is relevant here is twofold:

The GCC and Clang directives used in this PR to disable this warning will now disable it on all AIX builds, including JDK 8
JDK 8 on AIX, now like all other JDK builds on AIX, produces only this single JIT warning (previously, the different front end produced a set of 16 warnings that only appeared on AIX JDK 8 builds).

Therefore, disabling this warning would allow us to enable OMR_WARNINGS_AS_ERRORS unconditionally on Power, which is the last platform on which it is not yet enabled (ignoring obscure not-yet-fully-supported platforms like possibly RISC-V?).

@pshipton @zl-wang As the two people who were helping me out with this last year, could I get another round of review when you have a chance? Since it's been a while, we're disabling this warning instead of fixing it because it was decided that a fix would require a somewhat significant refactoring of multiple classes. In the meantime, it is worthwhile to disable this warning while work is done to perform the refactoring so that we can turn on OMR_WARNINGS_AS_ERRORS.

pshipton · 2024-09-19T20:15:38Z

Note IBM Java 8 still uses xlc 13.1, pls ensure this build isn't broken by this change.

dylanjtuttle · 2024-09-19T20:36:26Z

I see. I suppose I should retract some of the sweeping statements I made in my previous comment. That means a separate effort still needs to be applied to remove those xlC warnings so we can enable OMR_WARNINGS_AS_ERRORS unconditionally on Power.

I do think it would be worthwhile to disable this warning and turn on warnings as errors on non-xlC Power builds in the meantime. It would add another safety rail against bugs to all of our other Power builds sooner, rather than risking more being introduced while work is done to fix the warnings for IBM Java 8 on AIX. I could open an issue for xlC specific warnings so progress on that could be tracked seperately.

zl-wang · 2024-09-20T12:15:07Z

Note IBM Java 8 still uses xlc 13.1, pls ensure this build isn't broken by this change.

somehow need to make sure, if they stay at xlc13.1, don't upgrade up to xlc13.1.5 (my impression) where it regressed the performance (significantly).

pshipton · 2024-09-20T15:16:58Z

jenkins compile aix jdk8,jdk21

keithc-ca · 2024-09-20T17:27:15Z

runtime/compiler/control/J9Options.cpp

+#if defined(__clang__)
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Winvalid-offsetof"
+#elif defined(__GNUC__) || defined(__GNUG__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Winvalid-offsetof"
+#endif


It's not clear to me that the changes to this file will have any effect. The warnings need to be suppressed where SET_OPTION_BIT() is used, not where it is defined.

The -Winvalid-offsetof warnings are seen in the last nightly build https://openj9-jenkins.osuosl.org/job/Build_JDK21_ppc64_aix_Nightly/347
but not the PR build https://openj9-jenkins.osuosl.org/job/Build_JDK21_ppc64_aix_Personal/255

I see what Keith is talking about, because it doesn't look like this specific SET_OPTION_BIT() warning appears in the nightly build. I guess something must have happened since I was originally fixing this warning last year, and recently since I only checked to see if it still got rid of the warnings, I missed that one of them doesn't do anything anymore.

There are no warnings in the nightly that mention SET_OPTION_BIT, so I think that part of the change could be reverted.

dylanjtuttle force-pushed the disableOffsetOf branch from ccaa82b to 00ce5ab Compare November 1, 2023 19:24

dylanjtuttle force-pushed the disableOffsetOf branch from b2c3ff2 to 6070e12 Compare November 2, 2023 18:08

dylanjtuttle force-pushed the disableOffsetOf branch from 6070e12 to 68bc1a6 Compare November 3, 2023 18:41

dylanjtuttle force-pushed the disableOffsetOf branch from 68bc1a6 to 2d8c3ac Compare November 16, 2023 20:26

dylanjtuttle force-pushed the disableOffsetOf branch from 2d8c3ac to 69080b6 Compare December 6, 2023 14:50

dylanjtuttle requested a review from dsouzai as a code owner December 6, 2023 14:50

zl-wang reviewed Dec 6, 2023

View reviewed changes

dylanjtuttle force-pushed the disableOffsetOf branch 2 times, most recently from bb03f8e to be0a6d3 Compare December 7, 2023 20:26

pshipton reviewed Dec 7, 2023

View reviewed changes

dylanjtuttle force-pushed the disableOffsetOf branch from be0a6d3 to 06fdf70 Compare December 8, 2023 17:36

dylanjtuttle commented Dec 8, 2023

View reviewed changes

dylanjtuttle force-pushed the disableOffsetOf branch 2 times, most recently from 662e7bd to b842e49 Compare December 8, 2023 20:52

hzongaro mentioned this pull request May 15, 2024

gcc10 warnings in the JIT #14859

Open

dylanjtuttle force-pushed the disableOffsetOf branch from b842e49 to 0e884bc Compare June 11, 2024 18:02

dylanjtuttle force-pushed the disableOffsetOf branch from 0e884bc to d0811d4 Compare June 11, 2024 18:04

dylanjtuttle mentioned this pull request Sep 19, 2024

Refactor classes to have standard layout type #18328

Open

dylanjtuttle changed the title ~~Disable -Winvalid-offsetof in kca_offsets_generator and J9ValueProfiler~~ Disable -Winvalid-offsetof in J9Options, kca_offsets_generator, J9ValueProfiler Sep 19, 2024

pshipton approved these changes Sep 19, 2024

View reviewed changes

zl-wang approved these changes Sep 20, 2024

View reviewed changes

pshipton merged commit c8520fe into eclipse-openj9:master Sep 20, 2024
5 checks passed

keithc-ca reviewed Sep 20, 2024

View reviewed changes

dylanjtuttle mentioned this pull request Sep 20, 2024

Remove no-op disable of -Winvalid-offsetof in J9Options #20208

Merged

pshipton added the comp:jit label Sep 20, 2024

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

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

Conversation

dylanjtuttle commented Oct 19, 2023 • edited Loading

hzongaro commented Nov 1, 2023

zl-wang commented Nov 1, 2023

dylanjtuttle commented Nov 1, 2023 • edited Loading

dylanjtuttle commented Nov 2, 2023

zl-wang commented Nov 2, 2023

zl-wang commented Nov 2, 2023

dylanjtuttle commented Nov 2, 2023

zl-wang commented Nov 2, 2023

dylanjtuttle commented Nov 2, 2023

zl-wang commented Nov 3, 2023

dylanjtuttle commented Nov 3, 2023

zl-wang commented Nov 3, 2023

zl-wang commented Nov 3, 2023

dylanjtuttle commented Nov 3, 2023

dylanjtuttle commented Dec 5, 2023

zl-wang commented Dec 5, 2023

pshipton commented Dec 5, 2023

dylanjtuttle commented Dec 5, 2023

pshipton commented Dec 5, 2023

zl-wang commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanjtuttle Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanjtuttle commented Dec 12, 2023 • edited Loading

dylanjtuttle commented Sep 19, 2024 • edited Loading

pshipton commented Sep 19, 2024

dylanjtuttle commented Sep 19, 2024

zl-wang commented Sep 20, 2024

pshipton commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanjtuttle Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanjtuttle commented Oct 19, 2023 •

edited

Loading

dylanjtuttle commented Nov 1, 2023 •

edited

Loading

dylanjtuttle Dec 8, 2023 •

edited

Loading

dylanjtuttle commented Dec 12, 2023 •

edited

Loading

dylanjtuttle commented Sep 19, 2024 •

edited

Loading

dylanjtuttle Sep 20, 2024 •

edited

Loading