RFR(m): 8185712: [windows] Improve native symbol decoder
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Fri Sep 22 15:14:12 UTC 2017
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 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