RFR: 8252921: NMT overwrite memory type for region assert when building dynamic archive [v3]

[Ioi Lam]

On Thu, 17 Sep 2020 00:46:33 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

> > > > This code in
> > > > [virtualMemoryTracker.cpp](https://github.com/openjdk/jdk/blob/9a7dcdcdbad34e061a8988287fe691abfd4df305/src/hotspot/share/services/virtualMemoryTracker.cpp#L350)
> > > > still looks buggy to me: ```
> > > >     if (reserved_rgn->same_region(base_addr, size)) {
> > > >       reserved_rgn->set_call_stack(stack);
> > > >       reserved_rgn->set_flag(flag);
> > > >       return true;
> > > >     } else {
> > > >       assert(reserved_rgn->overlap_region(base_addr, size), "Must be");
> > > > 
> > > >       // Overlapped reservation.
> > > >       // It can happen when the regions are thread stacks, as JNI
> > > >       // thread does not detach from VM before exits, and leads to
> > > >       // leak JavaThread object
> > > >       if (reserved_rgn->flag() == mtThreadStack) {
> > > >         guarantee(!CheckJNICalls, "Attached JNI thread exited without being detached");
> > > >         // Overwrite with new region
> > > > ```
> > > > 
> > > > 
> > > > Why is the "Overlapped reservation" fix not done when the two regions are exactly the same?
> > > 
> > > 
> > > The same region branch was to deal with different scenario: recursive reservation, where os::reserve() ->
> > > pd_reserve() -> os::reserve()
> > > > If a JNI thread has exited without detaching, and its stack happens to be picked for a future memory reservation of the
> > > > exact size, I think we will get the same assert as in this bug report.
> > > 
> > > 
> > > Right, there is possibility. We can put the same guarantee there.
> > 
> > 
> > I think the order of the checks should be changed:
> > ```
> > if (reserved_rgn->overlap_region(base_addr, size)) {
> >     check for overlapped reservation
> > }
> > if (reserved_rgn->same_region(base_addr, size)) {
> >    check for recursive reservation
> > ```
> > 
> > 
> > In fact, if the code had been written this way, this bug would not have happened.
> 
> Well, I don't know which bug you refer to, but overlap_region() covers same_region(), so reversing order is a no go.

I am talking the original assert reported by [JDK-8252921](https://bugs.openjdk.java.net/browse/JDK-8252921)

assert((flag() == mtNone || flag() == f)) failed: Overwrite memory type for region
[0x000000a41cf40000-0x000000a41d040000), 3->23.

We get there because we have a old stack region which matches exactly as a newly reserved region. This case should also
be handled by this code // Overlapped reservation.
// It can happen when the regions are thread stacks, as JNI
// thread does not detach from VM before exits, and leads to
  

The current code assumes that if you have an exact match, it must be from a recursive reveration. This is wrong.

-------------

PR: https://git.openjdk.java.net/jdk/pull/185