RFR(m): 8185712: [windows] Improve native symbol decoder
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Sep 7 08:16:33 UTC 2017
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 )
On Wed, Sep 6, 2017 at 3:40 PM, <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> 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