[8u] RFR 8233197: Invert JvmtiExport::post_vm_initialized() and Jfr:on_vm_start() start-up order for correct option parsing

[Jaroslav Bachorík]

snip ...

I have reverted the change in the class initialization order and put
the JvmtiExport change in a guarded block as requested.
All tests are passing.

I have also experimented with using Jfr::is_disabled() which basically
translates to check whether -XX:-FlightRecorder is set but that makes
the original patch for 8233197 to fail because correct handling of an
attempt to initialize JFR from JVMTI_VM_EVENT_START handling requires
reordering of the JvmtiExport calls in thread.cpp - and that does not
happen if JVM is started with -XX:-FlightRecorder.

Perhaps, it would be possible to modify jfrJvmtiAgent.cpp to check
whether JFR is disabled before attempting to initialize the JFR agent
but that might turn out to be not the only place requiring adjustments
and that would lead to rather large diversion between the original fix
and the backport.

The latest webrev:
http://cr.openjdk.java.net/~jbachorik/8233197/hotspot/webrev.05

Cheers,

-JB-


On Wed, Jun 10, 2020 at 9:55 PM Andrew Hughes <gnu.andrew at redhat.com> wrote:
>
> On 08/06/2020 17:41, Jaroslav Bachorík wrote:
> > Hi Andrew,
> >
>
>
> snip...
>
> >>
> >> 1. How prolific is such usage of JFR?
> >
> > I guess not much since till recently each attempt would have resulted
> > in segfault.
> > But in general I would say that any APM/Profiler based on JFR would be
> > happy not to run a special delay code to prevent segfaults.
> >
>
> Ok, I guess that applies to JFR in later OpenJDK versions (i.e. those in
> releases) as well as 8u.
>
> >>
> >> 2. What are the risks of including this patch?
> >
> > In general, changing the initialization order for java.lang.Class
> > could cause problems for a code that would operate with assumption of
> > the java.lang.Class not being initialized very early on in the JVM
> > startup. This sounds pretty unlikely, though - eg tier1, tier2 and jfr
> > tests are still passing after adding this change
> >
>
> It's still pretty worrying to be altering this in a very stable release
> (though, adding JFR itself is worrying as well)
>
> I'll let Andrew Haley make the final call on whether he is happy to
> allow this or not.
>
> >
> >>
> >> 3. Do we now have a final version of this patch, with test?
> >
> > Yes. The webrev.04 version is the final one.
> >
> >
> >>
> >> 4. Was a bug filed for the additional test?
> >
> > https://bugs.openjdk.java.net/browse/JDK-8246703
> >
>
> Thanks for this follow-up.
>
> Cheers,
> --
> Andrew :)
>
> Senior Free Java Software Engineer
> Red Hat, Inc. (http://www.redhat.com)
>
> PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
> Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
>