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

[Zhengyu Gu]

On Thu, 17 Sep 2020 00:27:29 GMT, Ioi Lam <iklam 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.

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

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