(Nestmates) RFR (M): 8240645: Update nest host determination in line with latest proposed JVMS changes for JEP-371

[John Rose]

Nice work, very thorough as usual.

One nit.  The back-to-back parens are bad style.

> foo (bar)(baz)

https://english.stackexchange.com/questions/19429/should-i-use-adjacent-parentheses-or-a-semicolon-or-something-else
https://blog.apastyle.org/apastyle/2013/05/punctuation-junction-parentheses-and-brackets.html

There are different ways to handle this in professional text.
I think the simplest way is relatively tricky from our perspective.
You merge the two parentheticals with an internal semicolon:

> foo (bar; baz)

A really simple tweak to your code would be to put a comma
between.  This is less standard but better than back-to-back
parens:

-+        ss.print("(");
++        ss.print(“, (“);

(Two places.)

Likewise, in two places:

-+      msg = "(stuff…)";
++      msg = " (stuff…)”;

…to avoid the left paren making hard contact with the preceding text.

Klass::_nest_host_res_error is ok.  If we ever decide to reduce the
footprint there (as part of a larger effort, I think), we could merge
both fields by using a tagged pointer, on the observation that you
only need one or the other to be non-null.  If the res-error is not
null, then we can infer that nest-host has been determined to be
self, and in that case we don’t need to look at the field, right?
You might want to add a comment about the possible states of
those two coupled variables, to help maintainers.  For example:

+ // This variable is non-null only if _nest_host is null.

Actually, I didn’t follow the logic fully:  Can both fields be non-null
at the same time?  It looks like Klass::nest_host only gets set non-null
on error-free paths, while the error string is set only on error paths.
It would be reasonable for Klass::nest_host to check for an error
string and immediately return “this” if one is set.  I didn’t see this.

Maybe the comment should be:

+ // This variable is non-null only if _nest_host is this or (during a race) null.

(And if there’s a race, please do a releasing store of the error string before
setting the _nest_host to “this”.  But at-least-one-always-null is a safer
convention for managing races.  So I like the first comment much better.)

Also, let’s talk about VirtualMachineError.

After thinking about it a bit, I think you went too far replacing CATCH
by THREAD; this makes a fragile assumption that nothing is coming
out of the call.  It’s fragile because, although we expect LinkageError,
there’s usually an additional possibility of VirtualMachineError.
Doing a catch-and-clear of VME is risky business, and in this case
I only think we need to catch and clear the LE, not the VME.

To me, it seems safest to always expect VME, and always pass it on,
as a basic convention in our code base, with clear documentation
wherever we do something different.

Imagine getting an access error later on, with the parenthetical
remark that the nest host failed due to SOE or OOME.  Those guys
should be fail-fast, I think.  Or did you come up with a reason they
should be muzzled also, along with the LE we intend to muzzle?

I see the previous code processed VME differently, and that you
removed it, so I know you did this as a considered step.  But still
it seems off to me.  At least add some documentation explaining
why it’s safe to drop VM errors.

— John