RFR(m): 8185712: [windows] Improve native symbol decoder
Thomas Stüfe
thomas.stuefe at gmail.com
Wed Sep 6 13:16:32 UTC 2017
Great, thank you!
On Wed, Sep 6, 2017 at 3:11 PM, Lindenmaier, Goetz <
goetz.lindenmaier at 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 at gmail.com]
> > Sent: Mittwoch, 6. September 2017 14:38
> > To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> > Cc: hotspot-runtime-dev at openjdk.java.net; Ioi Lam <ioi.lam at oracle.com>;
> > Zhengyu Gu <zgu at 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 at sap.com <mailto:goetz.lindenmaier at 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-
> > native-symbol-resolver/webrev.04/webrev/index.html
> >
> >
> > Delta:
> > 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 at gmail.com
> > <mailto:thomas.stuefe at gmail.com> ]
> > > Sent: Dienstag, 5. September 2017 15:06
> > > To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com
> > <mailto:goetz.lindenmaier at sap.com> >
> > > Cc: hotspot-runtime-dev at openjdk.java.net <mailto:hotspot-
> > runtime-dev at openjdk.java.net> ; Ioi Lam <ioi.lam at oracle.com
> > <mailto:ioi.lam at 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-
> > improve- <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-
> > improve- <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 at sap.com
> > <mailto:goetz.lindenmaier at sap.com> <mailto:goetz.lindenmaier at sap.com
> > <mailto:goetz.lindenmaier at 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> - 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-> >
> > > > bounces at openjdk.java.net
> > <mailto:bounces at openjdk.java.net> <mailto:bounces at openjdk.java.net
> > <mailto:bounces at openjdk.java.net> > ]
> > > On Behalf Of Thomas Stüfe
> > > > Sent: Mittwoch, 30. August 2017 14:34
> > > > To: hotspot-runtime-dev at openjdk.java.net <mailto:
> hotspot-
> > runtime-dev at openjdk.java.net> <mailto:hotspot- <mailto:hotspot->
> > > runtime-dev at openjdk.java.net <mailto:runtime-
> > dev at 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> >
> > > > 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.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> > )
> >
> > > >
> > > > -------------
> > > >
> > > > 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
> > >
> > >
> >
> >
> >
>
>
More information about the hotspot-runtime-dev
mailing list