-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
8345569: [ubsan] adjustments to filemap.cpp and virtualspace.cpp for macOS aarch64 #22603
Conversation
👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into |
@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:
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 97 new commits pushed to the
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 |
Webrevs
|
@@ -2245,7 +2245,7 @@ address FileMapInfo::heap_region_requested_address() { | |||
// Runtime base = 0x4000 and shift is also 0. If we map this region at 0x5000, then | |||
// the value P can remain 0x1200. The decoded address = (0x4000 + (0x1200 << 0)) = 0x5200, | |||
// which is the runtime location of the referenced object. | |||
return /*runtime*/ CompressedOops::base() + r->mapping_offset(); | |||
return /*runtime*/ (address)((uintptr_t)CompressedOops::base() + r->mapping_offset()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If base()
can return 0 (nullptr) then how does the casting help with the warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the addition on uintptr_t is defined in C++ while the addition on nullptr is not.
That's why the ubsan warning/error goes away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I misread the placement of the parentheses.
So that sounds like signed arithmetic is being performed instead of unsigned. |
I think the virtualspace.cpp related issue came in just recently . Earlier last week I did not see this issue. |
There is another addition to nullptr in Warning/error is We should address this here as well because it is in the same file and similar to the other code location. |
Should I adjust the title of the PR + JBS issue ? It does not match well any more what is done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine to me.
Feel free to update the JBS/PR title if you choose.
Thanks
Hi David, thanks for the review ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nit. Otherwise, LGTM. Thanks for fixing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
failing GHA for linux-x64 has
so that's most likely unrelated. |
/integrate |
Going to push as commit 1d2ccae.
Your commit was automatically rebased without conflicts. |
This fixes the build when building on macOS aarch64 with ubsan enabled.
Seems there is an undefined addition to a nullptr in filemap.cpp :
jdk/src/hotspot/share/cds/filemap.cpp:2215:47: runtime error: applying non-zero offset 34358689792 to null pointer
#0 0x107b70c78 in FileMapInfo::heap_region_requested_address() filemap.cpp:2215
#1 0x107b71960 in FileMapInfo::map_heap_region_impl() filemap.cpp:2260
#2 0x107b70e04 in FileMapInfo::map_or_load_heap_region() filemap.cpp:2081
#3 0x1082976ec in MetaspaceShared::map_archives(FileMapInfo*, FileMapInfo*, bool) metaspaceShared.cpp:1344
#4 0x10829699c in MetaspaceShared::initialize_runtime_shared_and_meta_spaces() metaspaceShared.cpp:1098
#5 0x108289530 in Metaspace::global_initialize() metaspace.cpp:736
#6 0x108819da8 in universe_init() universe.cpp:887
#7 0x107d8b4ec in init_globals() init.cpp:133
#8 0x1087e43d8 in Threads::create_vm(JavaVMInitArgs*, bool*) threads.cpp:574
#9 0x107eca96c in JNI_CreateJavaVM jni.cpp:3681
#10 0x102e6e770 in JavaMain java.c:494
#11 0x102e7579c in ThreadJavaMain java_md_macosx.m:679
#12 0x19d38ef90 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x6f90)
#13 0x19d389d30 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1d30)
coding in filemap.cpp is (and CompressedOops::base() seems to return nullptr on this macoS aarch64 machine)
return /*runtime*/ CompressedOops::base() + r->mapping_offset();
This was seen in the OpenJDK build on macOS aarch64 when building with ubsan enabled.
There is also another very recent issue showing up in the ubsan enabled build on macOS aarch64 since today.
jdk/src/hotspot/share/memory/virtualspace.cpp:462:18: runtime error: applying non-zero offset to non-null pointer 0x000080000000 produced null pointer
#0 0x10a6a2df0 in ReservedHeapSpace::try_reserve_range(char*, char*, unsigned long, char*, char*, unsigned long, unsigned long, unsigned long) virtualspace.cpp:462
#1 0x10a6a3684 in ReservedHeapSpace::initialize_compressed_heap(unsigned long, unsigned long, unsigned long) virtualspace.cpp:569
#2 0x10a6a39cc in ReservedHeapSpace::ReservedHeapSpace(unsigned long, unsigned long, unsigned long, char const*) virtualspace.cpp:647
#3 0x10a6a3bd0 in ReservedHeapSpace::ReservedHeapSpace(unsigned long, unsigned long, unsigned long, char const*) virtualspace.cpp:622
#4 0x10a625d5c in Universe::reserve_heap(unsigned long, unsigned long) universe.cpp:959
#5 0x1099d4580 in G1CollectedHeap::initialize() g1CollectedHeap.cpp:1286
#6 0x10a6255e8 in universe_init() universe.cpp:880
#7 0x109b95dec in init_globals() init.cpp:133
#8 0x10a5efd98 in Threads::create_vm(JavaVMInitArgs*, bool*) threads.cpp:574
#9 0x109cd53b4 in JNI_CreateJavaVM jni.cpp:3680
#10 0x104d26770 in JavaMain java.c:494
#11 0x104d2d79c in ThreadJavaMain java_md_macosx.m:679
#12 0x19d38ef90 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x6f90)
#13 0x19d389d30 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1d30)
... (rest of output omitted)
For now I exclude this method from ubsan checking.
After these changes, the build on macOS aarch64 with ubsan enabled works .
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22603/head:pull/22603
$ git checkout pull/22603
Update a local copy of the PR:
$ git checkout pull/22603
$ git pull https://git.openjdk.org/jdk.git pull/22603/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 22603
View PR using the GUI difftool:
$ git pr show -t 22603
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22603.diff
Using Webrev
Link to Webrev Comment