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

Get rid of AIX linker option: -bnorwexec #63

Merged
merged 1 commit into from
May 29, 2024

Conversation

zl-wang
Copy link
Contributor

@zl-wang zl-wang commented May 29, 2024

High-level: this linker option is oxymoronic with JIT environment (where executable code are generated on the fly anyway).

At least for the time being before we figured out why default codeCache is allocated through malloc (instead of mmap or shmget, for which that linker option appeared to have no effect by my tests).

Even after we figured it out eventually, it is a legitimate question what value this option provides for JIT environment.

this also has effect on FFI thunks (which are not generated in JIT codecache).

Issue ibmruntimes/openj9-openjdk-jdk21#162

@pshipton
Copy link
Member

The IBM copyright end date needs to be updated.

High-level: this linker option is oxymoronic with JIT environment (where executable code are generated on the fly anyway).

At least for the time being before we figured out why default codeCache is allocated through malloc (instead of mmap or shmget, for which that linker option appeared to have no effect by my tests).

Even after we figured it out eventually, it is a legitimate question what value this option provides for JIT environment.

this also has effect on FFI thunks (which are not generated in JIT codecache).

Issue ibmruntimes/openj9-openjdk-jdk21#162
@keithc-ca
Copy link
Member

keithc-ca commented May 29, 2024

A similar change should be proposed for https://github.com/ibmruntimes/openj9-openjdk-jdk first.
Nevermind, it was removed in ibmruntimes/openj9-openjdk-jdk@61b1e26.

@@ -86,7 +86,7 @@ AC_DEFUN([FLAGS_SETUP_LDFLAGS_HELPER],
LDFLAGS_CXX_PARTIAL_LINKING="$MACHINE_FLAG -r"

if test "x$OPENJDK_TARGET_OS" = xaix; then
BASIC_LDFLAGS="-Wl,-b64 -Wl,-brtl -Wl,-bnorwexec -Wl,-bnolibpath -Wl,-bnoexpall \
BASIC_LDFLAGS="-Wl,-b64 -Wl,-brtl -Wl,-bnolibpath -Wl,-bnoexpall \
Copy link
Member

Choose a reason for hiding this comment

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

We don't use clang on AIX, so this should make any difference (yet?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, i think we switched to xlC16's xlclang already. for java23, OpenXL(LLVM based) is expected.

Copy link
Member

Choose a reason for hiding this comment

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

No, we're still using the xlc toolchain; see the most recent acceptance build: https://openj9-jenkins.osuosl.org/job/Build_JDKnext_ppc64_aix_OpenJDK/672.

[2024-05-29T04:26:54.412Z] configure: Using default toolchain xlc (IBM XL C/C++)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see later:
BUILD_ENV_VARS_LIST = [PATH+XLC=/opt/IBM/xlC/16.1.0/bin:/opt/IBM/xlc/16.1.0/bin, CC=xlclang, CXX=xlclang++]
That is clang too. plus, the head guard is ==xaix, and it is an linker option (linker is the same between xlC and OpenXL). It has the same effect.

Copy link
Member

Choose a reason for hiding this comment

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

The presence of "clang" in the names of the tools isn't relevant, the toolchain name, "xlc", is the reason the changed code is unused (see line 82):

  elif test "x$TOOLCHAIN_TYPE" = xclang; then

@keithc-ca keithc-ca merged commit 0d26e17 into ibmruntimes:openj9 May 29, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants