RFR(m): 8185712: [windows] Improve native symbol decoder

Wed Sep 6 12:37:30 UTC 2017

Hi Goetz,

On Wed, Sep 6, 2017 at 10:18 AM, Lindenmaier, Goetz <
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]
> > Sent: Dienstag, 5. September 2017 15:06
> > To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> > Cc: hotspot-runtime-dev at openjdk.java.net; Ioi Lam <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-
> > native-symbol-resolver/webrev.02
> >
> >
> > Delta to last:
> > 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> >
> > 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 - 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->
> >       > 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>
> >       > 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>
> >       > 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.01/webrev/
> >       >
> >       > (This is the followup to:
> > 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
> >
> >
>
>