[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: RE: RFR(m): 8185712: [windows] Improve native symbol decoder
From: "Lindenmaier, Goetz" <goetz.lindenmaier () sap ! com>
Date: 2017-09-22 15:21:47
Message-ID: 3a235712c9ca496185319cce5e85ba80 () sap ! com
[Download RAW message or body]
Hi Coleen,
that's great, thanks.
... I saw the other mail about jprt :) Daniel, Calvin thanks
for working on this!
Best regards,
Goetz
> -----Original Message-----
> From: coleen.phillimore@oracle.com [mailto:coleen.phillimore@oracle.com]
> Sent: Freitag, 22. September 2017 17:14
> To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>
> Cc: hotspot-runtime-dev@openjdk.java.net; Thomas Stüfe
> <thomas.stuefe@gmail.com>
> Subject: Re: RFR(m): 8185712: [windows] Improve native symbol decoder
>
>
> I would be happy to once the jdk10/hs repo is open. There are still
> problems with testing it.
>
> Thanks,
> Coleen
>
> On 9/22/17 8:56 AM, Lindenmaier, Goetz wrote:
> > Hi Coleen,
> >
> > I updated Thomas' webrev to the new repo structure.
> > I also ran it through our testing again.
> > Would you mind sponsoring it?
> > http://cr.openjdk.java.net/~goetz/wr17/8185712-windows-improve-
> native-symbol-resolver/webrev.05/
> >
> > Thanks,
> > Goetz.
> >
> > original webrev:
> > http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows-improve-
> native-symbol-resolver/webrev.04/webrev/
> >
> >
> >> -----Original Message-----
> >> From: Thomas Stüfe [mailto:thomas.stuefe@gmail.com]
> >> Sent: Donnerstag, 7. September 2017 10:17
> >> To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>
> >> Cc: hotspot-runtime-dev@openjdk.java.net; Coleen Phillmore
> >> <coleen.phillimore@oracle.com>
> >> Subject: Re: RFR(m): 8185712: [windows] Improve native symbol decoder
> >>
> >> Hi Goetz,
> >>
> >> as I am gone for vacation the next four weeks, could you please prepare
> the
> >> webrev rebased to the new repo once it is open and give it to Coleen?
> >>
> >> Thank you!
> >>
> >> (Last valid version was
> >> http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows-improve-
> >> native-symbol-resolver/webrev.04/webrev/index.html
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows-
> improve-
> >> native-symbol-resolver/webrev.04/webrev/index.html> )
> >>
> >> On Wed, Sep 6, 2017 at 3:40 PM, <coleen.phillimore@oracle.com
> >> <mailto:coleen.phillimore@oracle.com> > wrote:
> >>
> >>
> >>
> >> I will sponsor this for you, but remind me.
> >> Thanks,
> >> Coleen
> >>
> >>
> >>
> >> On 9/6/17 9:16 AM, Thomas Stüfe wrote:
> >>
> >>
> >> Great, thank you!
> >>
> >> On Wed, Sep 6, 2017 at 3:11 PM, Lindenmaier, Goetz <
> >> goetz.lindenmaier@sap.com
> >> <mailto:goetz.lindenmaier@sap.com> > wrote:
> >>
> >>
> >>
> >> HI Thomas,
> >>
> >> thanks for removing all that useless code. Looks
> >> perfect now :)
> >>
> >> Best regards,
> >> Goetz.
> >>
> >>
> >>
> >> -----Original Message-----
> >> From: Thomas Stüfe
> >> [mailto:thomas.stuefe@gmail.com <mailto:thomas.stuefe@gmail.com> ]
> >> Sent: Mittwoch, 6. September 2017 14:38
> >> To: Lindenmaier, Goetz
> >> <goetz.lindenmaier@sap.com <mailto:goetz.lindenmaier@sap.com> >
> >> Cc: hotspot-runtime-dev@openjdk.java.net
> >> <mailto:hotspot-runtime-dev@openjdk.java.net> ; Ioi Lam
> >> <ioi.lam@oracle.com <mailto:ioi.lam@oracle.com> >;
> >> Zhengyu Gu <zgu@redhat.com
> >> <mailto:zgu@redhat.com> >
> >> Subject: Re: RFR(m): 8185712: [windows]
> >> Improve native symbol decoder
> >>
> >> Hi Goetz,
> >>
> >> On Wed, Sep 6, 2017 at 10:18 AM,
> >> Lindenmaier, Goetz
> >> <goetz.lindenmaier@sap.com
> >> <mailto:goetz.lindenmaier@sap.com>
> <mailto:goetz.lindenmaier@sap.com
> >> <mailto:goetz.lindenmaier@sap.com> > >
> >> wrote:
> >>
> >>
> >> Hi Thomas,
> >>
> >> I had a look at the new webrev you sent
> >> after Zhengyu's comments.
> >> I appreciate the new tests. Looks good.
> >>
> >> I still think removal of
> >> Decoder::can_decode_C_frame_in_vm()
> >> should
> >> go into this change, because windows was
> >> the only platform to use
> >> this.
> >> If you insist put it in a change of its own,
> >> but to me it seems
> >>
> >>
> >> odd to
> >>
> >>
> >> leave
> >> this in the code in your change.
> >>
> >> Best regards,
> >> Goetz.
> >>
> >>
> >>
> >> Okay, you convinced me. I removed both
> >> Decoder::can_decode_C_frame_in_vm() and
> >> Decoder::shutdown() as you
> >> suggested in your earlier review.
> >>
> >> New Webrev:
> >>
> >>
> >> http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows-
> >> improve- <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-
> windows-
> >> improve->
> >> native-symbol-
> >> resolver/webrev.04/webrev/index.html
> >>
> >>
> >> Delta:
> >>
> >> http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows-
> >> improve- <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-
> windows-
> >> improve->
> >> native-symbol-resolver/webrev.03-to-
> >> 04/webrev/index.html
> >>
> >>
> >> Note to other reviewers: This new webrev
> >> just removes dead code, it
> >>
> >>
> >> should
> >>
> >>
> >> not have any function change over
> >> webrev.03.
> >>
> >> I did build on Linux x64, Aix, MacOS and
> >> Windows (32/64bit) and ran
> >>
> >>
> >> gtests on
> >>
> >>
> >> these platforms. Will run jtreg tests tonight.
> >>
> >> Thanks, Thomas
> >>
> >>
> >>
> >>
> >> > -----Original Message-----
> >> > From: Thomas Stüfe
> >> [mailto:thomas.stuefe@gmail.com <mailto:thomas.stuefe@gmail.com>
> >> <mailto:thomas.stuefe@gmail.com
> >> <mailto:thomas.stuefe@gmail.com> > ]
> >> > Sent: Dienstag, 5. September 2017 15:06
> >> > To: Lindenmaier, Goetz
> >> <goetz.lindenmaier@sap.com <mailto:goetz.lindenmaier@sap.com>
> >> <mailto:goetz.lindenmaier@sap.com
> >> <mailto:goetz.lindenmaier@sap.com> > >
> >> > Cc: hotspot-runtime-
> >> dev@openjdk.java.net <mailto:hotspot-runtime-dev@openjdk.java.net>
> >> <mailto:hotspot- <mailto:hotspot->
> >> runtime-dev@openjdk.java.net
> >> <mailto:runtime-dev@openjdk.java.net> > ; Ioi Lam <ioi.lam@oracle.com
> >> <mailto:ioi.lam@oracle.com>
> >> <mailto:ioi.lam@oracle.com
> >> <mailto:ioi.lam@oracle.com> > >
> >> > Subject: Re: RFR(m): 8185712: [windows]
> >> Improve native symbol
> >> decoder
> >> >
> >> > Hi Goetz,
> >> >
> >> > thank you for your review!
> >> >
> >> > New Webrev:
> >> >
> >> http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows-
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows->
> >> improve-
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows-
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows->
> >> improve->
> >> > native-symbol-resolver/webrev.02
> >> >
> >> >
> >> > Delta to last:
> >> >
> >> http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows-
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows->
> >> improve-
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows-
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows->
> >> improve->
> >> > native-symbol-resolver/webrev.01-to-
> >> 02/webrev/
> >> >
> >> >
> >> > The only change is that I removed the -
> >> XX:InitializeDbgHelpEarly
> >> switch to
> >> > avoid having to file a CSR.
> >> >
> >> > Please find further comments inline:
> >> >
> >> >
> >> > On Mon, Sep 4, 2017 at 5:08 PM,
> >> Lindenmaier, Goetz
> >> > <goetz.lindenmaier@sap.com
> >> <mailto:goetz.lindenmaier@sap.com>
> >> <mailto:goetz.lindenmaier@sap.com
> >> <mailto:goetz.lindenmaier@sap.com> >
> >> <mailto:goetz.lindenmaier@sap.com
> <mailto:goetz.lindenmaier@sap.com>
> >> <mailto:goetz.lindenmaier@sap.com
> >> <mailto:goetz.lindenmaier@sap.com> > > >
> >>
> >> > wrote:
> >> >
> >> >
> >> > Hi Thomas,
> >> >
> >> > I had a look at your change. Great
> >> somebody finally fixes
> >> > the windows symbol printing, thanks
> >> a lot!
> >> >
> >> > The code looks good, I'm just not sure
> >> whether you
> >> > need new files symbolengine.c|hpp.
> >> Isn't that
> >> > just what should go to
> >> decoder_windows.h|cpp and
> >> > class Decoder?
> >> > You would also get rid of the
> >> redirections in
> >> decoder_windows.cpp.
> >> >
> >> >
> >> >
> >> >
> >> > As we discussed, I see your point, but
> >> would prefer to leave the
> >> change for
> >> > the moment as it is.
> >> >
> >> > A similar change to this one - doing away
> >> with the
> >>
> >>
> >> AbstractDecoder
> >>
> >>
> >> object
> >> > instantiation layer - will be coming for
> >> AIX, where it does not
> >>
> >>
> >> make
> >>
> >>
> >> much
> >> > sense either, and I propose to do a
> >> separate cleanup or
> >> simplification change
> >> > once that is done, merging
> >> decoder_windows.cpp and
> >> > symbolengine.cpp/hpp. Unless I hear
> >> more objections from other
> >> reviewers,
> >> > I'd prefer to do this in a later patch.
> >> >
> >> >
> >> >
> >> > In shutdown() you comment
> >> > // There is no reason ever to shut
> >> down the decoder.
> >> > ... I think you can remove that
> >> function altogether, i.e.
> >>
> >>
> >> also
> >>
> >>
> >> > from the shared code, I don't see
> >> where it is ever called.
> >> >
> >> >
> >> >
> >> >
> >> > Totally agree...
> >> >
> >> >
> >> > Also, I think, you can just delete
> >> >
> >> Decoder::can_decode_C_frame_in_vm()
> >> > from the code. The only place where
> >> it is used, in
> >>
> >>
> >> frame.cpp,
> >>
> >>
> >> > calls dll_address_to_duntion_name().
> >> This returns useful
> >> information
> >> > also in the case of the NullDecoder,
> >> which now is the only
> >>
> >>
> >> one to
> >>
> >>
> >> > return false in that function.
> >> >
> >> >
> >> >
> >> > totally agree also here, but would also
> >> prefer both issues in a
> >> separate
> >> > change. In fact, Ioi opened a bug for this
> >> a while ago:
> >> >
> >> https://bugs.openjdk.java.net/browse/JDK-8144855
> >> <https://bugs.openjdk.java.net/browse/JDK-8144855>
> >> <https://bugs.openjdk.java.net/browse/JDK-
> >> 8144855 <https://bugs.openjdk.java.net/browse/JDK-8144855> > - and I
> >> would like
> >>
> >>
> >> to
> >>
> >>
> >> fix
> >> > it under that bug. Reason is, in this
> >> change, I'd like to avoid
> >>
> >>
> >> changing
> >>
> >>
> >> shared
> >> > sources as much as possible and keep
> >> this change windows only.
> >> >
> >> >
> >> >
> >> > Globals_windows.hpp needs
> >> Copyright adaption, please.
> >> > This is not introduced by your change,
> >> but maybe
> >> > you can also fix the copyright in
> >> decoder.hpp, which
> >> > says " 1997, 2015, 2017" ... should only
> >> name two
> >> > years ...
> >> >
> >> >
> >> >
> >> >
> >> > Not needed anymore: since I removed
> >> the -
> >> XX:InitializeDbgHelpEarly switch,
> >> > globals_windows.hpp is reverted to its
> >> original state. Do you
> >>
> >>
> >> still
> >>
> >>
> >> want me to
> >> > fix the date?
> >> >
> >> > Thanks for the review work!
> >> >
> >> > ..Thomas
> >> >
> >> >
> >> > Best regards,
> >> > Goetz.
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> >> > > -----Original Message-----
> >> > > From: hotspot-runtime-dev
> >> [mailto:hotspot-runtime-dev- <mailto:hotspot-runtime-dev->
> >> <mailto:hotspot-runtime-dev-
> >> <mailto:hotspot-runtime-dev-> >
> >>
> >> > <mailto:hotspot-runtime-dev-
> >> <mailto:hotspot-runtime-dev-> <mailto:hotspot-runtime-dev-
> >> <mailto:hotspot-runtime-dev-> > >
> >> > > bounces@openjdk.java.net
> >> <mailto:bounces@openjdk.java.net>
> >> <mailto:bounces@openjdk.java.net
> >> <mailto:bounces@openjdk.java.net> >
> <mailto:bounces@openjdk.java.net
> >> <mailto:bounces@openjdk.java.net>
> >> <mailto:bounces@openjdk.java.net
> >> <mailto:bounces@openjdk.java.net> > > ]
> >> > On Behalf Of Thomas Stüfe
> >> > > Sent: Mittwoch, 30. August 2017
> >> 14:34
> >> > > To: hotspot-runtime-
> >> dev@openjdk.java.net <mailto:hotspot-runtime-dev@openjdk.java.net>
> >> <mailto:
> >>
> >>
> >> hotspot-
> >>
> >>
> >> runtime-dev@openjdk.java.net
> >> <mailto:runtime-dev@openjdk.java.net> > <mailto:hotspot-
> >> <mailto:hotspot-> <mailto:hotspot- <mailto:hotspot-> >
> >> > runtime-dev@openjdk.java.net
> >> <mailto:runtime-dev@openjdk.java.net> <mailto:runtime-
> <mailto:runtime-
> >> dev@openjdk.java.net
> >> <mailto:dev@openjdk.java.net> > >
> >> > > Subject: RFR(m): 8185712:
> >> [windows] Improve native symbol
> >> > decoder
> >> > >
> >> > > Hi all,
> >> > >
> >> > > May I please have reviews for the
> >> following change.
> >> > >
> >> > > Issue:
> >> https://bugs.openjdk.java.net/browse/JDK-8185712
> >> <https://bugs.openjdk.java.net/browse/JDK-8185712>
> >> <https://bugs.openjdk.java.net/browse/JDK-
> >> 8185712 <https://bugs.openjdk.java.net/browse/JDK-8185712> >
> >> >
> >> <https://bugs.openjdk.java.net/browse/JDK-8185712
> >> <https://bugs.openjdk.java.net/browse/JDK-8185712>
> >> <https://bugs.openjdk.java.net/browse/JDK-
> >> 8185712 <https://bugs.openjdk.java.net/browse/JDK-8185712> > >
> >> > > Webrev:
> >> > >
> >> http://cr.openjdk.java.net/~stuefe/webrevs/8185712-
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712->
> >> windows-
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows-
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows->
> >> > improve-
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712->
> >> windows-
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows-
> >> <http://cr.openjdk.java.net/~stuefe/webrevs/8185712-windows->
> >> > improve->
> >> > > native-symbol-
> >> resolver/webrev.01/webrev/
> >> > >
> >> > > (This is the followup to:
> >> >
> >> https://bugs.openjdk.java.net/browse/JDK-8186349
> >> <https://bugs.openjdk.java.net/browse/JDK-8186349>
> >> <https://bugs.openjdk.java.net/browse/JDK-
> >> 8186349 <https://bugs.openjdk.java.net/browse/JDK-8186349> >
> >> >
> >> <https://bugs.openjdk.java.net/browse/JDK-8186349
> >> <https://bugs.openjdk.java.net/browse/JDK-8186349>
> >> <https://bugs.openjdk.java.net/browse/JDK-
> >> 8186349 <https://bugs.openjdk.java.net/browse/JDK-8186349> > > )
> >>
> >> > >
> >> > > -------------
> >> > >
> >> > > Basically, this is a reimplementation
> >> of the layer
> >>
> >>
> >> around the
> >>
> >>
> >> > Windows
> >> > > Symbol API (the API used to resolve
> >> debug symbols). The
> >>
> >>
> >> old
> >>
> >>
> >> > > implementation
> >> > > had a number of errors and
> >> shortcomings which together
> >> caused
> >> > the
> >> > > Windows
> >> > > native symbol resolution (and
> >> hence callstacks in error
> >>
> >>
> >> logs) to
> >>
> >>
> >> be a
> >> > bit
> >> > > of a lottery. The aim of this
> >> reimplementation is to
> >>
> >>
> >> make the
> >>
> >>
> >> code
> >> > more
> >> > > robust and easier to maintain.
> >> > >
> >> > > The problems with the existing
> >> implementation are listed
> >>
> >>
> >> in
> >>
> >>
> >> detail
> >> > in the
> >> > > bug description.
> >> > >
> >> > > The new implementation:
> >> > >
> >> > > - uses the new centralized
> >> WindowsDbgHelper class, which
> >> wraps
> >> > the
> >> > > dbghelp.dll loading, introduced with
> >> JDK-8186349
> >> > >
> >> > > - Completely bypasses the "create
> >> two instances of
> >> > AbstractDecoder class
> >> > > and synchronize access to them"
> >> scheme in decoder.cpp. It
> >> does
> >> > not make
> >> > > sense for windows, where we have
> >> to synchronize each
> >>
> >>
> >> access
> >>
> >>
> >> to
> >> > the
> >> > > dbghelp.dll anyway - this is done
> >> one layer below in
> >> > WindowsDbgHelper. The
> >> > > static methods of the shared
> >> Decoder class now directly
> >>
> >>
> >> access
> >>
> >>
> >> the
> >> > static
> >> > > methods in the new SymbolEngine
> >> class, see
> >> > decoder_windows.cpp.
> >> > >
> >> > > - The layer wrapping the Symbol API
> >> lives in the new
> >> > symbolengine.cpp/hpp
> >> > > files. The coding takes care of
> >> properly initializing
> >>
> >>
> >> (once) the
> >>
> >>
> >> symbol
> >> > API
> >> > > and of assembling the pdb search
> >> path.
> >> > >
> >> > > - Pdb search path construction is
> >> changed: where before
> >>
> >>
> >> we
> >>
> >>
> >> just
> >> > added jdk
> >> > > and jvm bin directories, we now just
> >> add all directories
> >>
> >>
> >> of all
> >>
> >>
> >> loaded
> >> > DLLs
> >> > > (which, of course, include the jdk
> >> and jvm bin
> >>
> >>
> >> directories). That
> >>
> >>
> >> way
> >> > we
> >> > > have a high chance of catching pdb
> >> files of third party
> >>
> >>
> >> libraries,
> >>
> >>
> >> as
> >> > long
> >> > > as they follow the convention of
> >> putting the pdb files
> >>
> >>
> >> beside
> >>
> >>
> >> the
> >> > dlls.
> >> > > This means it is easier to analyse
> >> crashes where third
> >>
> >>
> >> party
> >>
> >>
> >> DLLs are
> >> > > involved.
> >> > >
> >> > > - On Windows, we now have source
> >> file and line number in
> >>
> >>
> >> the
> >>
> >>
> >> > callstack.
> >> > >
> >> > > - There is a new parameter,
> >> diagnostic and windows-only,
> >> > > called "InitializeDbgHelpEarly". That
> >> parameter is by
> >>
> >>
> >> default
> >>
> >>
> >> off. If
> >> > on,
> >> > > it causes the symbol engine to be
> >> initialized early,
> >>
> >>
> >> which
> >>
> >>
> >> increases
> >> > the
> >> > > chance of good callstacks later on
> >> (because the
> >>
> >>
> >> initialization
> >>
> >>
> >> does
> >> > not
> >> > > have to run in an error situation).
> >> > >
> >> > > - Added tests: gtests and a jtreg test
> >> which tests the
> >>
> >>
> >> callstack
> >>
> >>
> >> > printing.
> >> > > All tests windows only. There is no
> >> technical reason for
> >>
> >>
> >> making
> >>
> >>
> >> > them
> >> > > windows only, but I wanted to keep
> >> disturbances to other
> >> > platforms to a
> >> > > minimum and these kind of tests
> >> can be shaky.
> >> > >
> >> > > Thanks a lot for reviewing this!
> >> > >
> >> > > Kind Regards, Thomas
> >> >
> >> >
> >>
> >>
> >>
> >>
> >>
> >>
> >>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic