8360941: [ubsan] MemRegion::end() shows runtime error: applying non-zero offset 8388608 to null pointer #26216
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 181 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 |
openjdk
bot
changed the title
Webrevs
|
|
I adjusted the test and took 'some other address than null' . And removed the cast from the header. |
|
Taking 'some other address' than nullptr / 0 as suggested just fails (I worked yesterday with opt build where no failure was seen). |
|
@tschatzl , maybe you have a good suggestion for an address ? |
It needs to be at least page-aligned. I'm not finding any other alignment requirement just now, but there might So The other likely alignment possibility is I still think allocating the space might be safer. So os::malloc space for one more region than the test calls for, and |
| // does not access it. | ||
| MemRegion heap(nullptr, num_regions_in_test * G1HeapRegion::GrainWords); | ||
| int val = 1; | ||
| HeapWord* ptr = reinterpret_cast<HeapWord*>(&val); |
There was a problem hiding this comment.
The initial value for val doesn't matter, since we're using its address rather than value.
And the cast could be removed by changing the type of val, leading to something like this:
HeapWord val{};
HeapWord* ptr = align_up(&val, os::vm_page_size());
There was a problem hiding this comment.
I adjusted the coding .
|
Hi Kim, should I add you as contributor ? |
| const size_t szw = num_regions_in_test * G1HeapRegion::GrainWords; | ||
| const size_t sz = szw * BytesPerWord; | ||
| char* addr = os::reserve_memory(sz, mtTest); | ||
| MemRegion heap((HeapWord*)addr, szw); |
There was a problem hiding this comment.
So far as I can tell, there's no guarantee that os::reserve_memory will return an address with any
particular alignment. Since the earlier attempt with unaligned storage failed, it may only be by accident
that this isn't failing as well. We have os::reserve_memory_aligned, or could add an extra region to
the desired size and align up the result.
There was a problem hiding this comment.
os::reserve_memory addresses are always regular-page-aligned. But os::reserve_memory_aligned may be better here since I guess the addresses would better have been region-size-aligned, so aligned to G1HeapRegion::GrainWords. That could be larger than system page size.
There was a problem hiding this comment.
Too bad the only way to find out about that alignment behavior is to dig into the sources for all ports
and read the documentation for the underlying OS function. :(
|
|
||
| bot_storage->uncommit_regions(0, num_regions_in_test); | ||
| delete bot_storage; | ||
| os::release_memory(addr, szw); |
There was a problem hiding this comment.
Wrong size; release_memory takes bytes, not words. Use sz.
This explains also the crash on Windows you told me about in your mail. Was this patch also in the testing queue?
| const size_t szw = num_regions_in_test * G1HeapRegion::GrainWords; | ||
| const size_t sz = szw * BytesPerWord; | ||
| const size_t lpsz = os::large_page_size(); | ||
| char* addr = os::reserve_memory_aligned(sz, lpsz, mtTest); |
There was a problem hiding this comment.
| char* addr = os::reserve_memory_aligned(sz, lpsz, mtTest); | |
| char* addr = os::reserve_memory_aligned(sz, G1HeapRegion::GrainBytes, mtTest); |
@MBaesken you want to align the start address at region size; not sure the G1 code we call in this test cares, but it might.
| const size_t szw = num_regions_in_test * G1HeapRegion::GrainWords; | ||
| const size_t sz = szw * BytesPerWord; | ||
| char* addr = os::reserve_memory(sz, mtTest); | ||
| MemRegion heap((HeapWord*)addr, szw); |
There was a problem hiding this comment.
Too bad the only way to find out about that alignment behavior is to dig into the sources for all ports
and read the documentation for the underlying OS function. :(
|
/contributor add kimbarrett |
|
@MBaesken Syntax:
User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address. |
|
/contributor add kbarrett |
|
@MBaesken |
|
/contributor add stuefe |
|
@MBaesken |
|
Thanks for the reviews ! /integrate |
|
Going to push as commit ceb0c0f.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
When running HS test
gtest/GTestWrapper.java
with ubsan-enabled binaries on macOS aarch64, the following error is reported (did not see this so far on Linux but there we use gcc and it seems the gcc/ubsan checks are a bit more limited).
test/hotspot/gtest/gc/g1/test_freeRegionList.cpp:37: Failure
Death test: child_G1FreeRegionList_length_()
Result: died but not with expected exit code:
Terminated by signal 6 (core dumped)
Actual msg:
Seems the test_freeRegionList.cpp uses a special MemRegion starting at nullptr ; but this causes a bit of trouble when adding to start == nullptr .
So far I see this issue just in the gtest, seems other MemRegion objects do not start at nullptr .
Progress
Issue
Reviewers
Contributors
<[email protected]><[email protected]>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26216/head:pull/26216$ git checkout pull/26216Update a local copy of the PR:
$ git checkout pull/26216$ git pull https://git.openjdk.org/jdk.git pull/26216/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26216View PR using the GUI difftool:
$ git pr show -t 26216Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26216.diff
Using Webrev
Link to Webrev Comment