[8u] Request for approval for CR 8145788 - fix EnableTracing crash
Hohensee, Paul
hohensee at amazon.com
Fri Aug 24 16:16:40 UTC 2018
Thanks, Sean. Pushed.
On 8/24/18, 9:10 AM, "Seán Coffey" <sean.coffey at oracle.com> wrote:
Paul,
the JDK 8 Updates Project information page is here :
http://openjdk.java.net/projects/jdk8u/
It's a longer running Project and currently uses email to approve push
requests. You already have approval to push and are good for integration
of 8145788 into http://hg.openjdk.java.net/jdk8u/jdk8u-dev/hotspot
Regards,
Sean.
On 24/08/18 17:00, Hohensee, Paul wrote:
> I added noreg-trival to the bug report, and also jdk8u-fix-request per
>
> http://openjdk.java.net/projects/jdk-updates/approval.html
>
> The wiki also says that jdk8u-fix-yes must be added before the patch can be pushed. Is that still true, and if not, what needs to happen before a push?
>
> Thanks,
>
> Paul
>
> On 8/23/18, 11:31 AM, "jdk8u-dev on behalf of Seán Coffey" <jdk8u-dev-bounces at openjdk.java.net on behalf of sean.coffey at oracle.com> wrote:
>
> 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