8354686: [AIX] now ubsan is possible by JoKern65 · Pull Request #24667 · openjdk/jdk

JoKern65 · 2025-04-15T18:40:57Z

With the introduction of the open XL 17.1.2.13 Compiler and the runtime 17.1.3 as minimum requirement ubsan is supported for AIX now.
Unfortunately there has to be some adoptions.

Currently the test for vptr does not work and produces crashes when active, so we have to deactivate it. (If fixed by IBM there will be a follow up JBS)
ubsan introduces so much new symbols to every executable that we have to link with -bbictoc.
The llvm_symbolizer is not found out of the box, so we have to provide the full qualified llvm_symbolizer path in the __ubsan_default_options() function.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8354686: [AIX] now ubsan is possible (Bug - P4)

Reviewers

Matthias Baesken (@MBaesken - Reviewer)
Christoph Langer (@RealCLanger - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24667/head:pull/24667
$ git checkout pull/24667

Update a local copy of the PR:
$ git checkout pull/24667
$ git pull https://git.openjdk.org/jdk.git pull/24667/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24667

View PR using the GUI difftool:
$ git pr show -t 24667

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24667.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-04-15T18:50:47Z

👋 Welcome back jkern! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-04-15T18:51:36Z

@JoKern65 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8354686: [AIX] now ubsan is possible

Reviewed-by: mbaesken, clanger

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 224 new commits pushed to the master branch:

66358fa: 8355616: Incorrect ifdef in compilationMemoryStatistic.cpp
e7a4162: 8355717: Problem list tests until JDK-8355708 is fixed
3eaec04: 8355635: Do not collect C strings in C2 scratch buffer
... and 221 more: https://git.openjdk.org/jdk/compare/cec48ed270d3bdf704c389a091b42a32c2ed6440...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-04-15T18:52:22Z

@JoKern65 The following label will be automatically applied to this pull request:

build

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-04-15T18:54:55Z

Webrevs

01: Full - Incremental (39a976f0)
00: Full (ae22be6e)

erikj79 · 2025-04-15T19:42:40Z

make/autoconf/jdk-options.m4

  UBSAN_CFLAGS="$UBSAN_CHECKS -Wno-stringop-truncation -Wno-format-overflow -Wno-array-bounds -Wno-stringop-overflow -fno-omit-frame-pointer -DUNDEFINED_BEHAVIOR_SANITIZER"
  UBSAN_LDFLAGS="$UBSAN_CHECKS"
+  if test "x$TOOLCHAIN_TYPE" = "xclang" && test "x$OPENJDK_TARGET_OS" = "xaix"; then
+      UBSAN_CFLAGS="$UBSAN_CFLAGS -fno-sanitize=function,vptr -DLLVM_SYMBOLIZER=$(dirname $(dirname $CC))/tools/ibm-llvm-symbolizer"


I would recommend handling this tool path properly, by first trying to look it up with one of the standard macros and then falling back to a default, and let configure fail if it wasn't found. This would also implicitly add the ability for the user to specify a different path if ever needed (using VAR_NAME=/path/to/tool on the command line). However, since this isn't a platform I'm supporting, I won't force you.

Can you please give me an example to try?

If the tool is (in case it is available) always at THIS position then it is not needed to add more logic. On the other hand, what happens if it is not there ? Would we get just less complete stacks in case of ubsan issues?

We would just get a stack without symbol resolution (not very helpful) and the ubsan message that the symbolizer could not be found.

okay thanks.
So back to my first question - is the tool always at this position ?

For the 17.1.2 Compiler it is always at the same position relative to the compiler executable itself. Of course you can install the 17.1.2 Compiler at a different absolute place, but this does not break the relative position to the compiler executable.
I do not know if it is stored at the same relative position in future versions of the compiler.

Thanks for clarifying! For 17.1.1 the ubsan stuff does not work anyway. For 17.1.2 it is the same (relative) position. So I guess we can leave it as it is ; in case XLC 18 or 19 has another tool or other fs layout of its binaries we have to deal with this anyway .

MBaesken · 2025-04-17T11:00:24Z

Please adjust COPYRIGHT header in ubsan_default_options.c.
And I think a comment line in the m4 and c file would not hurt.

MBaesken · 2025-04-25T08:15:34Z

When doing a ubsan enabled build on AIX I also noticed the following

warning: unknown warning option '-Wno-stringop-truncation'; did you mean '-Wno-string-concatenation'? [-Wunknown-warning-option]
warning: unknown warning option '-Wno-format-overflow'; did you mean '-Wno-shift-overflow'? [-Wunknown-warning-option]
warning: unknown warning option '-Wno-stringop-overflow'; did you mean '-Wno-shift-overflow'? [-Wunknown-warning-option]
3 warnings generated.
warning: unknown warning option '-Wno-stringop-truncation'; did you mean '-Wno-string-concatenation'? [-Wunknown-warning-option]
warning: unknown warning option '-Wno-format-overflow'; did you mean '-Wno-shift-overflow'? [-Wunknown-warning-option]
warning: unknown warning option '-Wno-stringop-overflow'; did you mean '-Wno-shift-overflow'? [-Wunknown-warning-option]
3 warnings generated.

Seems a couple of warning related flags that are set when running an ubsan enabled build do not exist on AIX (or generally with clang ?).
Maybe we should address/adjust this too. See line 523 in jdk-options.m4 just above your patch;
probably those options came from gcc ?

MBaesken · 2025-04-28T14:24:39Z

The warnings will be addressed in a follow up
https://bugs.openjdk.org/browse/JDK-8355594
8355594: Warnings occur when building with clang and enabling ubsan

JoKern65 · 2025-04-28T14:37:36Z

/integrate

openjdk · 2025-04-28T14:39:01Z

Going to push as commit b1e778d.
Since your change was applied there have been 224 commits pushed to the master branch:

66358fa: 8355616: Incorrect ifdef in compilationMemoryStatistic.cpp
e7a4162: 8355717: Problem list tests until JDK-8355708 is fixed
3eaec04: 8355635: Do not collect C strings in C2 scratch buffer
... and 221 more: https://git.openjdk.org/jdk/compare/cec48ed270d3bdf704c389a091b42a32c2ed6440...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-28T14:39:09Z

@JoKern65 Pushed as commit b1e778d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8354686

ae22be6

openjdk bot changed the title ~~JDK-8354686: [AIX] now ubsan is possible~~ 8354686: [AIX] now ubsan is possible Apr 15, 2025

openjdk bot added the rfr Pull request is ready for review label Apr 15, 2025

openjdk bot added the build build-dev@openjdk.org label Apr 15, 2025

erikj79 reviewed Apr 15, 2025

View reviewed changes

adding comments

39a976f

MBaesken approved these changes Apr 24, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 24, 2025

RealCLanger approved these changes Apr 28, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 28, 2025

openjdk bot closed this Apr 28, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 28, 2025

Conversation

JoKern65 commented Apr 15, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Apr 15, 2025

Uh oh!

openjdk bot commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 15, 2025

Uh oh!

mlbridge bot commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

erikj79 Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

JoKern65 Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

MBaesken Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

JoKern65 Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

MBaesken Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

JoKern65 Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

MBaesken Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

MBaesken commented Apr 17, 2025

Uh oh!

MBaesken commented Apr 25, 2025

Uh oh!

MBaesken commented Apr 28, 2025

Uh oh!

JoKern65 commented Apr 28, 2025

Uh oh!

openjdk bot commented Apr 28, 2025

Uh oh!

openjdk bot commented Apr 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

JoKern65 commented Apr 15, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Apr 15, 2025 •

edited

Loading

mlbridge bot commented Apr 15, 2025 •

edited

Loading