8334618: ubsan: support setting additional ubsan check options

[MBaesken]

Sometimes it would be helpful to have configure-support for adding additional ubsan check options.
E.g. support new configure option '--with-additional-ubsan-checks=' .

---

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-8334618: ubsan: support setting additional ubsan check options (Bug - P4)

Reviewers

• Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to ad51358a
• Lutz Schmidt (@RealLucy - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19802

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mbaesken! 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]

@MBaesken 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:

8334618: ubsan: support setting additional ubsan check options

Reviewed-by: stuefe, lucy

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 73 new commits pushed to the master branch:

• b2ac725: 8327380: Add tests for Shenandoah barrier expansion optimization
• f23295e: 8334600: TEST java/net/MulticastSocket/IPMulticastIF.java fails on linux-aarch64
• 741a0f3: 8334241: Adjust API docs side bar dimensions
• 4ce8822: 8334037: Local class creation in lambda in pre-construction context crashes javac
• 7f6804c: 8334872: BigEndian: java/lang/invoke/condy Tests failing since JDK-8294960
• e139005: 8333994: NMT: call stacks should show source information
• b88af94: 8269870: PS: Membar in PSPromotionManager::copy_unmarked_to_survivor_space could be relaxed
• a5f401f: 8334650: Add debug information about whether an Assertion Predicate is for the init or last value
• 25c3845: 8333133: Simplify QuickSort::sort
• c66f785: 8334505: RISC-V: Several tests fail when MaxVectorSize does not match VM_Version::_initial_vector_length
• ... and 63 more: https://git.openjdk.org/jdk/compare/642084629a9a793a055cba8a950fdb61b7450093...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]

@MBaesken 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]

Webrevs

• 03: Full - Incremental (0b7402e0)
• 02: Full - Incremental (ad51358a)
• 01: Full - Incremental (6246d85a)
• 00: Full (7c8cba89)

[TheShermanTanker]

I'm not very fond of adding new configure options, but I guess this is ok. A thought, rather than a raw AC_ARG_WITH, newer code should try to adopt UTIL_ARG_WITH instead, for example:

UTIL_ARG_WITH(NAME: additional-ubsan-checks, TYPE: string, DEFAULT: [], DESC: [Custom ubsan checks], OPTIONAL: true)

UBSAN_CHECKS="-fsanitize=undefined -fsanitize=float-divide-by-zero -fno-sanitize=shift-base -fno-sanitize=alignment $ADDITIONAL_UBSAN_CHECKS"

More information about UTIL_ARG_WITH and friends can be found in their implementation documentation here:

jdk/make/autoconf/util.m4

Line 579 in 5cad0b4

# Creates a command-line option using the --with-* pattern. Will return a

[MBaesken]

Thanks for the advice, UTIL_ARG_WITH makes the change much smaller.

[erikj79]

Logic looks ok, but I would prefer if you could break up the lines a bit.

[MBaesken]

I adjusted the lines a bit.

[kimbarrett]

[Just a drive-by comment, not a review and not planning to review.]
As a user, syntactically I think I'd prefer something like the with-ubsan[=parameters] approach suggested in the
the probably duplicate issue https://bugs.openjdk.org/browse/JDK-8306710. But that's not a strongly held opinion.
And I have no idea what would be better for the implementation.

[tstuefe]

Stupid question, could I not just pass --with-extra-cxxflags or similar, and have the same effect?

[MBaesken]

Stupid question, could I not just pass --with-extra-cxxflags or similar, and have the same effect?

Technically probably yes, but wouldn't that be cflags AND cxxflags ?
Another positive point with this PR - the added settings show up on the command line together with the other ubsan flags.

[MBaesken]

[Just a drive-by comment, not a review and not planning to review.] As a user, syntactically I think I'd prefer something like the with-ubsan[=parameters] approach suggested in the the probably duplicate issue https://bugs.openjdk.org/browse/JDK-8306710. But that's not a strongly held opinion. And I have no idea what would be better for the implementation.

Your idea is more flexible (you can not just add something but overwrite the existing settings).
However you have to set more ubsan parameters on the other hand, so both approaches have pros and cons.

[tstuefe]

OK

[erikj79]

We could have both --with-ubsan-checks= and --with-additional-ubsan-checks if you think it would be useful. Without having any experience using ubsan, I would guess that there is rarely a case for reducing from the default set, so if going for just one, adding to the set seems more convenient. Overriding the set could be achieved using C/C++ flags directly, though more inconvenient.

[TheShermanTanker]

Note: My "Custom ubsan checks" description was just a placeholder for you to replace, I think the original description you had for it was better

[TheShermanTanker]

I should also add that it's been quite a while since I implemented the fix for the UTIL_XXX utilities, does using UTIL_ARG_WITH in this case work in the way you want it to? If not, I might need to take a look at them again in case they've regressed since the fix

[MBaesken]

We could have both --with-ubsan-checks= and --with-additional-ubsan-checks if you think it would be useful.

What are the preferences of the others ? Yeah it might be useful indeed (Kim proposed a flag for replacing the checks in the other JBS issue).

[RealLucy]

To me, changes look good.

[MBaesken]

Thanks for the reviews !
The langtools error in 'Pre-submit tests - linux-x64 / test - Test (tier1) ' seems to be unrelated.

/integrate

[openjdk]

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

• b2ac725: 8327380: Add tests for Shenandoah barrier expansion optimization
• f23295e: 8334600: TEST java/net/MulticastSocket/IPMulticastIF.java fails on linux-aarch64
• 741a0f3: 8334241: Adjust API docs side bar dimensions
• 4ce8822: 8334037: Local class creation in lambda in pre-construction context crashes javac
• 7f6804c: 8334872: BigEndian: java/lang/invoke/condy Tests failing since JDK-8294960
• e139005: 8333994: NMT: call stacks should show source information
• b88af94: 8269870: PS: Membar in PSPromotionManager::copy_unmarked_to_survivor_space could be relaxed
• a5f401f: 8334650: Add debug information about whether an Assertion Predicate is for the init or last value
• 25c3845: 8333133: Simplify QuickSort::sort
• c66f785: 8334505: RISC-V: Several tests fail when MaxVectorSize does not match VM_Version::_initial_vector_length
• ... and 63 more: https://git.openjdk.org/jdk/compare/642084629a9a793a055cba8a950fdb61b7450093...master

Your commit was automatically rebased without conflicts.

[openjdk]

@MBaesken Pushed as commit efb905e.

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