RFR(m): 8185712: [windows] Improve native symbol decoder
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Fri Sep 22 12:56:47 UTC 2017
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 at gmail.com]
> Sent: Donnerstag, 7. September 2017 10:17
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Cc: hotspot-runtime-dev at openjdk.java.net; Coleen Phillmore
> <coleen.phillimore at 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 at oracle.com
> <mailto:coleen.phillimore at 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 at sap.com
> <mailto: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 <mailto:thomas.stuefe at gmail.com> ]
> Sent: Mittwoch, 6. September 2017 14:38
> 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> >;
> Zhengyu Gu <zgu at redhat.com
> <mailto: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> <mailto: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- <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 at gmail.com <mailto:thomas.stuefe at gmail.com>
> <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>
> <mailto: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>
> <mailto:hotspot- <mailto:hotspot->
> runtime-dev at openjdk.java.net
> <mailto:runtime-dev at openjdk.java.net> > ; Ioi Lam <ioi.lam at oracle.com
> <mailto:ioi.lam at oracle.com>
> <mailto: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-
> <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 at sap.com
> <mailto:goetz.lindenmaier at sap.com>
> <mailto:goetz.lindenmaier at sap.com
> <mailto:goetz.lindenmaier at sap.com> >
> <mailto: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>
> <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 at openjdk.java.net
> <mailto:bounces at openjdk.java.net>
> <mailto:bounces at openjdk.java.net
> <mailto:bounces at openjdk.java.net> > <mailto: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-
>
>
> runtime-dev at openjdk.java.net
> <mailto:runtime-dev at openjdk.java.net> > <mailto:hotspot-
> <mailto:hotspot-> <mailto:hotspot- <mailto:hotspot-> >
> > runtime-dev at openjdk.java.net
> <mailto:runtime-dev at openjdk.java.net> <mailto:runtime- <mailto:runtime-
> >
> dev at openjdk.java.net
> <mailto: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> >
> >
> <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
> >
> >
>
>
>
>
>
>
>
More information about the hotspot-runtime-dev
mailing list