RFR(m): 8185712: [windows] Improve native symbol decoder
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Wed Sep 6 13:11:26 UTC 2017
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