RFR: 8251850: Simplify ResourceMark constructors using delegation

[Thomas Stüfe]

On Fri, Aug 21, 2020 at 6:26 PM Kim Barrett <kim.barrett at oracle.com> wrote:

> > On Aug 18, 2020, at 5:40 AM, Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
> >
> >
> >
> > On Tue, Aug 18, 2020 at 9:52 AM Kim Barrett <kim.barrett at oracle.com>
> wrote:
> > > On Aug 17, 2020, at 10:44 PM, David Holmes <david.holmes at oracle.com>
> wrote:
> > >
> > > Hi Kim,
> > >
> > > Not a full review - sorry. Have you tested what happens if a resource
> leak is introduced? The _warned variable was used, IIUC, to avoid hitting
> recursive errors during error reporting.
> >
> > OH!  The recursive case hadn’t occurred to me!
> >
> > Poorly named and poorly commented variable from before the beginning of
> the mercurial age,
> > so hard to track down the changeset that introduced it to see if that
> shed some light.  Good
> > thing we have some institutional memory :)
> >
> > I will do something about that, and then try to figure out how to
> trigger it.
> >
> > You could rig up something using -XX:ErrorHandlerTest
> (VMError::controlled_crash) and allocate from the same ResourceArea again
> inside the error handler. There are tests for similar things, e.g.
> SafeFetch, see hotspot/jtreg/runtime/ErrorReporting.
>
> Thanks for the testing hints.
>
> I've reinstated the check for resource allocation without a ResourceMark,
> though with more descriptive naming and comments.  Also dealt with the flag
> being potentially concurrently accessed, because I'm paranoid.
>
> I added a test for reporting of allocation without a ResourceMark.  I
> didn't
> add a test for the recursion case, though I hand-tested it.
>
> New webrevs:
> full: https://cr.openjdk.java.net/~kbarrett/8251850/open.02/
> incr: https://cr.openjdk.java.net/~kbarrett/8251850/open.02.inc/
>
> Ran tier1 testing.
>
>
Good.

+ // Only report the first occurrence of an allocating thread that
+ // is missing a ResourceMark, to avoid possible recursive errors
+ // in error handling.

The comment is clear. Should be still clear 40 years from now when we all
moved on from git and there is only one guy left who can read git history :)

Very minor nit: comment is somewhat imprecise here:

+    case 18: {
+      // Allocation from resource area without a ResourceMark.
+      Thread::current()->resource_area()->allocate_bytes(100);
+      break;
+    }

because this is only true if this function is called from argument handling
where no RM has been established yet.

I am fine with this change. I leave if up to you if you change the comment
or not.

Cheers, Thomas