Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add selected tracepoints for JCL #20936

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pshipton
Copy link
Member

pshipton added a commit to pshipton/openj9-openjdk-jdk8 that referenced this pull request Jan 14, 2025
@pshipton pshipton force-pushed the jcltracepoints branch 2 times, most recently from 4afe724 to a58c86b Compare January 14, 2025 20:52
pshipton added a commit to pshipton/openj9-openjdk-jdk8 that referenced this pull request Jan 14, 2025
@keithc-ca keithc-ca self-requested a review January 14, 2025 22:52
pshipton added a commit to pshipton/openj9-openjdk-jdk21 that referenced this pull request Jan 14, 2025
pshipton added a commit to pshipton/openj9-openjdk-jdk21 that referenced this pull request Jan 14, 2025
pshipton added a commit to pshipton/openj9-openjdk-jdk8 that referenced this pull request Jan 20, 2025
pshipton added a commit to pshipton/openj9-openjdk-jdk21 that referenced this pull request Jan 20, 2025
pshipton added a commit to pshipton/openj9-openjdk-jdk8 that referenced this pull request Jan 20, 2025
pshipton added a commit to pshipton/openj9-openjdk-jdk8 that referenced this pull request Jan 21, 2025
runtime/jcl/jcl_java.tdf Outdated Show resolved Hide resolved
pshipton added a commit to pshipton/openj9-openjdk-jdk8 that referenced this pull request Jan 23, 2025
pshipton added a commit to pshipton/openj9-openjdk-jdk21 that referenced this pull request Jan 23, 2025
runtime/include/jcltracepoints.h Outdated Show resolved Hide resolved
#endif /* defined(WIN32) */

#include "j9.h"
#include "tracehelp.c"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deserves a comment explaining why a header file includes a C source file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, if including tracehelp.c works, perhaps it should just appear in the .c file that includes jcltracepoints.h? It still feels wrong to include .c files in a .h file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I move tracehelp.c out, then jcltracepoints.h is no longer specific to tracepoints, but more generic for loading j9.h outside of OpenJ9. jcltracepoints may no longer be an appropriate name, does includej9h.h work for you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems somewhat awkward to me, but I'm not sure I have a better suggestion.

More troubling is the collection of configuration definitions: I think we should be including j9cfg.h instead (a number of source files in the extensions repos already do that).

Copy link
Member Author

@pshipton pshipton Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you talking about the parts like the following? I don't believe including j9cfg.h fixes these. The OpenJ9 definitions are on the command line, not defined in j9cfg.h afaik. I need to translate OpenJDK platform definitions to OpenJ9 definitions. Even j9cfg.h won't define the right things unless these are set.

#if defined(AIX)
#define AIXPPC
#define RS6000
#endif /* defined(AIX) */

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that j9.h does include j9cfg.h

@pshipton pshipton force-pushed the jcltracepoints branch 2 times, most recently from 03e3d52 to f4ca246 Compare January 23, 2025 19:23
pshipton added a commit to pshipton/openj9-openjdk-jdk21 that referenced this pull request Jan 23, 2025
pshipton added a commit to pshipton/openj9-openjdk-jdk8 that referenced this pull request Jan 23, 2025
Comment on lines 23 to 24
#if !defined(JCLTRACEPOINTS_H)
#define JCLTRACEPOINTS_H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure includej9.h is the best name for this header file; perhaps j9internals.h or j9access.h (hinting that it provides access to internal details of J9). Whatever we decide, the name of the macro here should be derived from the file name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to j9access.h

Comment on lines 37 to 47
#define COPY_PROGRESS_INFO_MASK 0
#if defined(AIX)
#define AIXPPC
#define RS6000
#endif /* defined(AIX) */
#if defined(MACOSX)
#define OSX
#endif /* defined(MACOSX) */
#if defined(WIN32)
#define OMR_OS_WINDOWS
#endif /* defined(WIN32) */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me that any of this should be necessary.

Copy link
Member Author

@pshipton pshipton Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were all added to fix compilation errors. Do you need me to retry builds without them and record the errors that occur? OpenJ9/OMR use different platform defines than OpenJDK uses/defines. If no matching OpenJ9 platform define is set, there are a couple places that define structures based on platform, and then a compilation error occurs because the structure isn't defined. That was the problem for AIX/Mac, for Windows it may be slightly different but along the same lines.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The errors when OSX is not defined below. I think they were the same for AIX but both RS600 and AIXPPC are needed to fix both of these.

/Users/jenkins/peter/openj9-openjdk-jdk8/openj9/runtime/oti/j9port.h:82:2: error: "J9PORT_LIBRARY_SUFFIX must be defined"
#error "J9PORT_LIBRARY_SUFFIX must be defined"
 ^
/Users/jenkins/peter/openj9-openjdk-jdk8/openj9/runtime/oti/j9port.h:324:2: error: unknown type name 'SYS_FLOAT'
        SYS_FLOAT cpuEntitlement;

Copy link
Member Author

@pshipton pshipton Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows without OMR_OS_WINDOWS:
c:\workspace\openj9-openjdk-jdk8\omr\include_core\omrport.h(55): fatal error C1083: Cannot open include file: 'unistd.h': No such file or directory

pshipton added a commit to pshipton/openj9-openjdk-jdk8 that referenced this pull request Jan 25, 2025
pshipton added a commit to pshipton/openj9-openjdk-jdk21 that referenced this pull request Jan 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants