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

[Ioi Lam]

On Thu, 17 Sep 2020 00:05:30 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.

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

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