[8u] Request for approval for CR 8145788 - fix EnableTracing crash
Seán Coffey
sean.coffey at oracle.com
Thu Aug 23 18:28:44 UTC 2018
This one looks pretty much like a straight backport of 8145788 from JDK 9.
Approved for jdk8u-dev. A suitable noreg- label [1] should be added to
the bug report.
regards,
Sean.
[1] http://openjdk.java.net/guide/changePlanning.html#noreg
On 23/08/2018 01:50, Liu, Xin wrote:
> Hi, JDK8u Developers
>
> Webrev: http://cr.openjdk.java.net/~phh/8145788/webrev.00/
> Problem: https://bugs.openjdk.java.net/browse/JDK-8145788
>
> I split the webrev. This one contains only the backport of 8145788.
> Could you approve it?
>
> @David,
> Thank for telling me the process and reviewing.
>
> Thanks,
> --lx
>
>
> On 8/21/18, 9:05 PM, "David Holmes" <david.holmes at oracle.com> wrote:
>
> Hi,
>
> On 22/08/2018 3:40 AM, Liu, Xin wrote:
> > Hi, David,
> >
> > New revision of CR:
> > http://cr.openjdk.java.net/~phh/8207913/webrev.02/
>
> If this is a clean direct backport of the change in 9 then this should
> be treated as a backport approval request. Close 8207913 as a duplicate
> and just import the changeset for 8145788 and request approval for that
> backport.
>
> You could use this bug to add the test but that would be a separate
> enhancement request (plus code review plus approval request). If you did
> add the test it would be preferable to do it all in Java using the test
> library and ProcessTools (I think we have that in 8u!) rather than a
> shell script.
>
> > It works. Because the main thread only reports Primitive Change Events, which don't use ResourceMark.
> > It's not 100% safe because it can't support to dump Klass * and Method * in main thread, or Thread::current() == NULL.
> > Does it matter? In my understanding, main thread sleeps until the java process is about to exit.
>
> There are lots of things that can fail if they are used too early in the
> VM initialization process - this is no different. Obviously to use a
> ResourceMark you must have an attached thread, and there is a period of
> time during VM initialization where the main thread is not yet attached
> and so Thread::current() will be NULL.
>
> FYI I'm on vacation after today so I won't be able to continue this
> discussion.
>
> Cheers,
> David
>
> > Thanks,
> > --lx
> >
> > On 8/21/18, 12:38 AM, "David Holmes" <david.holmes at oracle.com> wrote:
> >
> > Hi,
> >
> > On 21/08/2018 5:14 PM, Liu Xin wrote:
> > >
> > > hi, Dave,
> > >
> > > Thank you reviewing it. Indeed, I intend to submit to jdk8u-dev.
> > >
> > > I backtrace the change. here is the original bug.
> > > https://bugs.openjdk.java.net/browse/JDK-8032250
> >
> > Okay. So what you're actually looking for is a backport of this bug
> > fixed in 9:
> >
> > "8145788: JVM crashes with -XX:+EnableTracing"
> >
> > https://bugs.openjdk.java.net/browse/JDK-8145788
> >
> > Can you try the patch in 8u and see if that fixes the issue:
> >
> > http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/2abdc525b6b4
> >
> > Thanks,
> > David
> >
> >
> > > I found a person mentioned -XX:+EnableTracing was broken for OpenJdk8u
> > > in 2014. He said jdk7 is okay.
> > > http://hllvm.group.iteye.com/group/topic/42367
> > >
> > > You are right. I found some events do need ResourceMark. It's because
> > > they need to use NEW_RESOURCE_ARRAY in TraceStream. Actually, only 2
> > > cases really need to NEW_RESOURCE_ARRAY, Klass * and Method*.
> > >
> > > Is that okay that I use a fixed buffer on stack here? By using it, I
> > > can safely eliminate the ResourceMark in traceEventClasses.cpp.
> > >
> > > diff --git a/src/hotspot/src/share/vm/trace/traceStream.hpp
> > > b/src/hotspot/src/share/vm/trace/traceStream.hpp
> > > index 89911ea2..8d4a732f 100644
> > > --- a/src/hotspot/src/share/vm/trace/traceStream.hpp
> > > +++ b/src/hotspot/src/share/vm/trace/traceStream.hpp
> > > @@ -32,6 +32,8 @@
> > > #include "oops/symbol.hpp"
> > > #include "utilities/ostream.hpp"
> > >
> > > +const int MAX_LOCAL_BUFFER = 256;
> > > +
> > > class TraceStream : public StackObj {
> > > private:
> > > outputStream& _st;
> > > @@ -79,29 +81,24 @@ class TraceStream : public StackObj {
> > > _st.print("%s = %f", label, val);
> > > }
> > >
> > > - // Caller is machine generated code located in traceEventClasses.hpp
> > > - // Event<TraceId>::writeEvent() (pseudocode) contains the
> > > - // necessary ResourceMark for the resource allocations below.
> > > - // See traceEventClasses.xsl for details.
> > > void print_val(const char* label, const Klass* const val) {
> > > + char buf[MAX_LOCAL_BUFFER];
> > > const char* description = "NULL";
> > > +
> > > if (val != NULL) {
> > > Symbol* name = val->name();
> > > if (name != NULL) {
> > > - description = name->as_C_string();
> > > + description = name->as_C_string(buf, MAX_LOCAL_BUFFER);
> > > }
> > > }
> > > _st.print("%s = %s", label, description);
> > > }
> > >
> > > - // Caller is machine generated code located in traceEventClasses.hpp
> > > - // Event<TraceId>::writeEvent() (pseudocode) contains the
> > > - // necessary ResourceMark for the resource allocations below.
> > > - // See traceEventClasses.xsl for details.
> > > void print_val(const char* label, const Method* const val) {
> > > + char buf[MAX_LOCAL_BUFFER];
> > > const char* description = "NULL";
> > > if (val != NULL) {
> > > - description = val->name_and_sig_as_C_string();
> > > + description = val->name_and_sig_as_C_string(buf, MAX_LOCAL_BUFFER);
> > > }
> > > _st.print("%s = %s", label, description);
> > > }
> > > thanks,
> > > --lx
> > >
> > >
> > >
> > >
> > > On Mon, Aug 20, 2018 at 7:04 PM David Holmes <david.holmes at oracle.com
> > > <mailto:david.holmes at oracle.com>> wrote:
> > >
> > > Hi,
> > >
> > > I dropped jdk8-dev at openjdk.java.net
> > > <mailto:jdk8-dev at openjdk.java.net> and added
> > > jdk8u-dev at openjdk.java.net <mailto:jdk8u-dev at openjdk.java.net> to
> > > the cc line.
> > >
> > > On 21/08/2018 8:38 AM, Liu, Xin wrote:
> > > > Hi, Runtime develop,
> > > >
> > > > Could you review this change basing on jdk8u?
> > > > Problem: https://bugs.openjdk.java.net/browse/JDK-8207913
> > > > Webrev: http://cr.openjdk.java.net/~phh/8207913/webrev.01/
> > > <http://cr.openjdk.java.net/%7Ephh/8207913/webrev.01/>
> > > >
> > > > JDK8u has this problem because Thread::current() is NULL when it
> > > handles global arguments at startup. OracleJDK8 doesn’t suffer from
> > > this issue.
> > > > I found jdk12’s counterpart jfrEventClasses.hpp doesn’t have
> > > ResourceMark. Removing it works for both with/without ttyLocker.
> > >
> > > The ResourceMark has been there since the file was introduced in JDK 8.
> > > What has changed to cause this problem and when? If we remove the RM
> > > then we have to be sure no events that get traced will in fact rely on
> > > there being a RM present.
> > >
> > > > We will check in jdk8u/hotspot if reviewers approve it.
> > >
> > > There is a specific approval process that has to be followed after the
> > > change has been Reviewed.
> > >
> > > http://openjdk.java.net/projects/jdk8u/approval-template.html
> > >
> > > Thanks,
> > > David
> > >
> > > > Thanks,
> > > > --lx
> > > >
> > >
> >
> >
>
>
More information about the jdk8u-dev
mailing list