👋 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.

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@MBaesken The following labels will be automatically applied to this pull request:

• client
• core-libs

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

Webrevs

• 03: Full - Incremental (0ada81d1)
• 02: Full - Incremental (cfbf904b)
• 01: Full - Incremental (fb17d1bc)
• 00: Full (a2f89ecf)

jni_md.h seems like a pretty big sledgehammer for something that seems to only be in one file, for just java.desktop. Is this macro planned to be used outside of java.desktop?

jni_md.h seems like a pretty big sledgehammer for something that seems to only be in one file, for just java.desktop. Is this macro planned to be used outside of java.desktop?

Yes I have a couple (currently 2) more I would like to exclude as well .
The exclusions should work (similar to HS) for the whole JDK C codebase.

The exclusions should work (similar to HS) for the whole JDK C codebase.

Maybe we can offer a separate header ub.hpp next to jni_md.h that contains the macro/attribute stuff.
Similar to sanitizers/ub.hpp in HS. This header is then included in the few files were the macro/attributes are needed.

jni_md.h seems like a pretty big sledgehammer for something that seems to only be in one file, for just java.desktop. Is this macro planned to be used outside of java.desktop?

Yes I have a couple (currently 2) more I would like to exclude as well . The exclusions should work (similar to HS) for the whole JDK C codebase.

Alright, if this is planned to be used across the entire JDK and not just in java.desktop then I guess this should be ok. Indeed, perhaps a ubsan.h header would be appropriate for a macro like this. I'll leave that up to further discussion

/reviewers 3 reviewers

This needs careful thinking about by various parties.
I am not sold on it, and the build team have indicated they do not support --enable-ubsan and my experience with that and similar options is that they are a nightmare to find a system on which they build and produce a working binary.

Until there's solid committed support by the leads of the build team I don't think we should be considering these kinds of PRs.

@prrace
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 3 Reviewers).

build team have indicated they do not support --enable-ubsan

What 'build team' are you talking about ? Some Oracle internal build team?
The ubsan support came in 2023, so nothing new (change was from Justin King)
7a85d95
And it was reviewed by erikj and ihse, I think those are members of the OpenJDK build group and very competent OpenJDK reviewers.
So the flag is as good or bad as any other configure flag of OpenJDK .

and my experience with that and similar options is that they are a nightmare to find a system on which they build

We use and run this regularly on SUSE and Ubuntu Linux and have VERY good experience with it. It works (at least on recent Linux distros) very well. (on macOS our experience is a bit different there I had a couple of issues with ubsan)
A lot of large C/C++ based OSS projects support it like Linux kernel, Android, Chromium etc.
https://docs.kernel.org/dev-tools/ubsan.html
https://source.android.com/docs/security/test/ubsan
https://www.chromium.org/developers/testing/undefinedbehaviorsanitizer/

and btw. Oracle blogs happily about it too and recommends it
https://blogs.oracle.com/linux/post/improving-application-security-with-undefinedbehaviorsanitizer-ubsan-and-gcc
So it is a well established tool in the OSS world.

And it was reviewed by erikj and ihse, I think those are members of the OpenJDK build group

Erik and Magnus are indeed part of the build team. Erik is the Skara tool lead but does build work as well, while Magnus is pretty much the face of the build team at this point

A lot of large C/C++ based OSS projects support it like Linux kernel, Android, Chromium etc.

I do wish MSYS2/gcc supported it as well. It could help me catch Windows bugs too

very competent OpenJDK reviewers

If only I could be a competent reviewer one of these days :(

Btw regarding the function/method exclusion, this was introduced in HS some time ago
https://bugs.openjdk.org/browse/JDK-8334239
We just needed in HS codebase a couple of places where we use/keep undefined behavior (e.g. im vmError.cpp where it is intentionally used). Obviously, the mentioned HS change does not work for the other C libs so this is some kind of follow up.
In HS we use the ATTRIBUTE_NO_UBSAN macro currently at 4 code locations. So it is not intended or likely that this macro would go to a lot of places of the JDK libs, just a small number comparable to usage in HS.
Regarding putting it into jni_md.h, probably it would moving it from there to a separate header like it is done in HS would be better, I can do that if this is preferred.

jni_md.h is shipped as part of every JDK distribution - this change does NOT belong in that file.

jni_md.h is shipped as part of every JDK distribution - this change does NOT belong in that file.

Hi David, should I introduce a separate ub.hpp (similar to what we have in Hotspot) ?

Hi David, should I introduce a separate ub.hpp (similar to what we have in Hotspot) ?

As previously discussed above a separate header would be best, though I'm not sure where it would be placed if you are using this across code in different modules.

@MBaesken my understanding is that src/java.base/share/native/include is for exported header files, it is not a general location to put this new header.

@MBaesken my understanding is that src/java.base/share/native/include is for exported header files, it is not a general location to put this new header.

Seems the include directories
src/java.base/unix/native/libjava
and also hotspot/os/posix/include and src/hotspot/share/include
show up in the '-I' settings of these C compilation units in jdk. And from what I see those are not exported
Should I move the header to some of these ?
Putting it into some hotspot location sounds odd because we have already 'sanitizers/ub.hpp' in hotspot.
So maybe src/java.base/unix/native/libjava ?

hotspot/share/include contains the headers that hotspot exports to the jdk. os/posix/include/ has the platform specific header that hotspot exports to the jdk.

So maybe src/java.base/unix/native/libjava

That is header files for libjava.

This is why I said it would be hard to find a shared location where this can be used across different modules - because there presently isn't one.

So maybe src/java.base/unix/native/libjava

That is header files for libjava.

This is why I said it would be hard to find a shared location where this can be used across different modules - because there presently isn't one.

In practise the headers from there go into a lot of libs across the whole jdk C codebase.
Take for example jni_util.h (share/native/libjava/jni_util.h) - I see this additionally to libjava in C code from libmanagement, awt, librmi , libnio just to name a few .

So maybe jni_util.h / jni_util_md.h itself is a place where this can be put. Or a header located beside it .
(other examples : 'io_util.h' , this is included too in libzip ;
'jdk_util.h' - included in libawt, libnet, libinstrument, ... )

jni_util.h is used across all modules but it is located in java.base/share/native/libjava not java.base/unix/native/libjava.

I think you could probably place ub.h along-side jni_util.h in that directory.

I think you could probably place ub.h along-side jni_util.h in that directory.

I moved the header to the better location.

@prrace

the build team have indicated they do not support --enable-ubsan and my experience with that and similar options is that they are a nightmare to find a system on which they build and produce a working binary.

The UBSan functionality is well supported, as such, in the build system. As you say, actually building and running a JDK with UBSan functionality is not trivial. This just means that we are unlikely to e.g. run it continuously on our CI in the foreseeable future, not that any changes towards improving support for detecting undefined behavior should be stopped.

As a general note, we have discussed multiple times if, or rather how, we could have a generic library that is shared by native code across all Java modules, and Hotspot code alike, but no satisfactory solution has been found as of yet.

In this particular case, where all that is needed is an additional header file, I think the currently suggested solution of putting it in libjava makes sense. The majority of other JDK native libraries already depend on libjava.

The UBSan functionality is well supported, as such, in the build system. As you say, actually building and running a JDK with
UBSan functionality is not trivial. This just means that we are unlikely to e.g. run it continuously on our CI

We are running it for some weeks now at least once a week in our central builds/tests (so far only on Linux x86_64).
If you have a recent SUSE or Ubuntu ubsan works nicely with OpenJDK . But I haven't checked on all popular distros.
In the past we had just too many ubsan-related issues in the OpenJDK codebase that even hindered doing a build, but this situation has improved a lot over the last months.

We are running it for some weeks now at least once a week in our central builds/tests (so far only on Linux x86_64).

That is good to hear. Kudos to you for all your hard work on getting it there!

jni_util.h is used across all modules but it is located in java.base/share/native/libjava not java.base/unix/native/libjava.

I think you could probably place ub.h along-side jni_util.h in that directory.

Hi David, are you fine with the latest version ?

I do wonder why the header both here and inside HotSpot is named ub.h instead of ubsan.h. Latter sounds better to me, but it doesn't really matter much

I agree it would have probably been better from the start, but at this point consistency with hotspot is more important.

In the review of the HS PR ubsan.hpp was discussed
#19722
but then it was changed to ub.hpp .

Not a review, just commenting.

I very much support the work @MBaesken has done with ubsan in HotSpot.
There's a bit of a chicken or egg problem with this kind of thing. ubsan
isn't really usable and in any way supported or supportable until the code has
been made pretty much ubsan-clean. Matthias has made substantial progress
toward that for HotSpot, and I thank him for this effort.

There are currently four uses of the disabling attribute in HotSpot. Two are
for intentional things (writing to address zero intentionally, for testing
signal handling and the like). One is for a known issue that is being worked
on, with an associated JBS issue; the attribute is being used to reduce
testing noise in the meantime. The fourth is related to some of my recent
work (adjacent to, not caused by), and something that I think ought to be
fixed. I'll be filing a JBS issue. Along the way there have been a
substantial number of real bugs uncovered and addressed.

My only complaint has been a tendency to be a bit too quick to reach for the
disabling attribute, without sufficient analysis and attempt to resolve in a
way that corrects the problem. Of the issues I've observed, the result of a
real fix nearly always ben a straight-up improvement.