RFR(s): 8186349: [windows] Centralize dbghelp handling code
Ioi Lam
ioi.lam at oracle.com
Thu Aug 24 15:37:54 UTC 2017
OK, I will run a bunch of windows 32/64 tests in our test environment to
make sure things are OK, and then I'll push.
Thanks
- Ioi
On 8/24/17 2:15 AM, Thomas Stüfe wrote:
> Thanks Goetz!
>
> @Ioi: We are good now for reviews. Thanks for sponsoring!
>
> ..Thomas
>
> On Thu, Aug 24, 2017 at 11:14 AM, Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com <mailto:goetz.lindenmaier at sap.com>> wrote:
>
> Looks perfect now, Reviewed.
>
> Best,
> Goetz.
>
> > -----Original Message-----
> > From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com
> <mailto:thomas.stuefe at gmail.com>]
> > Sent: Donnerstag, 24. August 2017 10:51
> > To: Ioi Lam <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>>;
> Reingruber, Richard
> > <richard.reingruber at sap.com
> <mailto:richard.reingruber at sap.com>>; 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>
> > Subject: Re: RFR(s): 8186349: [windows] Centralize dbghelp
> handling code
> >
> > Hi guys,
> >
> > thanks for the reviews!
> > @Ioi: thanks for sponsoring!
> >
> >
> > New Webrev:
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8186349-centralize-dbghelp-
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8186349-centralize-dbghelp->
> > handling/webrev.02/webrev/
> >
> >
> > Delta to last:
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8186349-centralize-dbghelp-
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8186349-centralize-dbghelp->
> > handling/webrev.01-to-02/webrev/
> >
> >
> > Nothing exciting changed. I addressed all of Goetz and Richards
> concerns and
> > added a Summary line to the change.
> >
> > Change built and tested (gtests only) on Windows x86, x64.
> >
> > Kind Regards, Thomas
> >
> >
> >
> > On Wed, Aug 23, 2017 at 4:26 PM, 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>> > wrote:
> >
> >
> > Hi Thomas, thanks for addressing my concerns. I can
> sponsor the
> > change after you get OK from the other reviewers.
> >
> > Thanks
> >
> > - Ioi
> >
> >
> >
> > On 8/22/17 5:48 AM, Thomas Stüfe wrote:
> >
> >
> > Hi all,
> >
> > please see new Webrev:
> >
> > http://cr.openjdk.java.net/~stuefe/webrevs/8186349-
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8186349->
> > centralize-dbghelp-handling/webrev.01/webrev/
> >
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8186349-centralize-dbghelp-
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8186349-centralize-dbghelp->
> > handling/webrev.01/webrev/>
> >
> >
> > I worked in proposed changes by Ioi and Richard.
> Sorry,
> > because on file name changed, I did not do an incremental
> webrev. Here are
> > the changes:
> >
> > - Renamed DbgHelpLoader to WindowsDbgHelp and renamed
> > the files too.
> > - I moved the critical section code to the
> implementation of
> > WindowsDbgHelp. I discarded the general "CritSectLocker" object
> in favour of
> > an object specialized for this case, see the EntryGuard class.
> > - I readded the SymSetOptions call I accidentally
> discarded.
> > - In decoder_windows.cpp, I set the value for
> > _can_decode_in_vm to true. I plan to remove this flag in one of
> the next
> > changes (see also JDK-8144855)
> >
> >
> > As Richard is no full reviewer, I'll need a second
> Reviewer and
> > a sponsor.
> >
> > Thanks!
> >
> > Kind Regards, Thomas
> >
> >
> > On Tue, Aug 22, 2017 at 2:33 PM, Thomas Stüfe
> > <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>
> <mailto:thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>>
> > wrote:
> >
> >
> > Hi Richard,
> >
> >
> > thank you for the review! Please find my
> remarks
> > inline.
> >
> > On Tue, Aug 22, 2017 at 9:44 AM,
> Reingruber, Richard
> > <richard.reingruber at sap.com <mailto:richard.reingruber at sap.com>
> <mailto:richard.reingruber at sap.com
> <mailto:richard.reingruber at sap.com>> > wrote:
> >
> >
> > Hi Thomas,
> >
> > thanks for the refactoring work!
> >
> > I had a look at your changes, but
> please note
> > that I'm not a reviewer.
> >
> > ### dbghelp_loader.cpp:
> >
> > Should globalDefinitions.hpp be
> included?
> >
> >
> >
> > Currently I do not need it, so I'd rather not.
> >
> >
> >
> >
> > Little inconsistency: opening
> curly braces of
> > method bodies should be on the same line as the end of the
> parameter list, I
> > guess.
> >
> > 294: st->print("%s" #functionname,
> > ((num_missing > 0) ? ", " : ""));
> >
> >
> >
> > Are you sure? Sorry, I cannot spot an
> error here.
> >
> >
> >
> >
> > Format string is incomplete.
> >
> > 196 BOOL
> > DbgHelpLoader::stackWalk64(DWORD MachineType,
> > 197 HANDLE
> hProcess,
> > 198 HANDLE hThread,
> > 199 LPSTACKFRAME64
> > StackFrame,
> > 200 PVOID
> > ContextRecord)
> > 201 {
> > 202 CritSectLocker lck(&g_cs);
> > 203 if
> (initialize_if_needed()) {
> > 204 if (g_pfn_StackWalk64
> != NULL) {
> > 205 return
> > g_pfn_StackWalk64(MachineType, hProcess, hThread, StackFrame,
> > 206 ContextRecord,
> > 207 NULL, //
> > ReadMemoryRoutine
> > 208
> > g_pfn_SymFunctionTableAccess64, // FunctionTableAccessRoutine,
> > 209
> > g_pfn_SymGetModuleBase64, // GetModuleBaseRoutine
> > 210 NULL //
> > TranslateAddressRoutine
> > 211 );
> > 212 }
> > 213 }
> > 214 return FALSE;
> > 215 }
> >
> > Lines 208, 209: is it ok to pass NULL?
> >
> >
> >
> >
> > Good question. Documentation says
> parameters are
> > required. I tested calling this with NULL for both functions and
> stack walking
> > worked just fine. I leave it as it is, because I think at worst
> we risk StackWalk64
> > failing, and at best we get a callstack nevertheless.
> >
> >
> >
> > ### windows_decoder.cpp
> >
> > The following line was deleted without
> > replacement. Are the options not needed?
> >
> > SymSetOptions(SYMOPT_UNDNAME |
> > SYMOPT_DEFERRED_LOADS | SYMOPT_EXACT_SYMBOLS);
> >
> >
> >
> >
> > Good catch! Will fix.
> >
> >
> >
> >
> > Cheers, Richard.
> >
> >
> >
> >
> > Thanks!
> >
> >
> > Thomas
> >
> >
> > -----Original Message-----
> > From: hotspot-runtime-dev
> [mailto:hotspot- <mailto:hotspot->
> > runtime-dev-bounces at openjdk.java.net
> <mailto:runtime-dev-bounces at openjdk.java.net>
> <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: Freitag, 18. August 2017 09:24
> > 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:hotspot-runtime-dev at openjdk.java.net>>
> > Subject: RFR(s): 8186349:
> [windows] Centralize
> > dbghelp handling code
> >
> > Dear all,
> >
> > may I please have a review for
> this change:
> >
> > Issue:
> > 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>>
> > Webrev:
> > http://cr.openjdk.java.net/~stuefe/webrevs/
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/>
> > <http://cr.openjdk.java.net/%7Estuefe/webrevs/
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/>>
> > 8186349-centralize-dbghelp-
> > handling/webrev.00/webrev/
> >
> > This is a part of an ongoing work
> I do to make
> > error reporting (especially
> > callstacks) on Windows more reliable.
> >
> > At first I did a rather large
> patch, see:
> > https://bugs.openjdk.java.net/
> > browse/JDK-8185712 and
> >
> > http://mail.openjdk.java.net/pipermail/hotspot-
> <http://mail.openjdk.java.net/pipermail/hotspot->
> > runtime-dev/2017-August/024286.html
> >
> <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-
> <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017->
> > August/024286.html>
> > . But after discussing this patch
> with Ioi, I saw
> > that this patch is better
> > split up into multiple parts for
> easier reviewing.
> >
> > So this is the first split up patch.
> >
> > --
> >
> > This patch here centralizes
> handling of the
> > dbghelp.dll (loading the
> > library, resolving function
> pointers and
> > synchronizing access).
> >
> > Which solves the problem that
> accesses to
> > functions exported from the
> > dbghelp.dll need to be
> synchronized, as stated
> > by the MSDN. We somehow
> > never really cared. I guess it
> never caused
> > visible trouble, because most
> > of the time (not always) the
> functions are
> > accessed from VMError::report(),
> > and chances of parallel access
> from other non-
> > error-reporting threads are
> > slim. Even if it were to crash,
> secondary error
> > handling would step in and
> > write an "Error occurred during error
> > reporting" or "Another thread had an
> > error too" message and we would
> probably just
> > shrug it off.
> >
> > But as this whole effort is about
> increasing the
> > chance of useful
> > callstacks in hs-err files, I'd
> like to fix this.
> >
> > In addition to the fix, I think
> this is also a nice
> > cleanup and removes
> > duplicate code.
> >
> > Notes:
> >
> > 1) Robustness: We may or may not
> find a
> > dbghelp.dll on the target system.
> > If we find it, it may be old or
> new (it is not
> > tightly coupled with the OS,
> > may be part of other installation
> packages, may
> > exist multiple times etc).
> > We should handle older versions of
> the dbghelp
> > dll gracefully and hide all
> > that complexity from the caller.
> >
> > 2) The new DbgHelpLoader class
> does not
> > export any state indicating whether
> > or not it successfully loaded, and
> if it loaded
> > which functions are
> > actually available. That was a
> deliberate
> > decision, there is no need for
> > the caller to know this. Caller
> should invoke the
> > DbgHelpLoader functions
> > as if they were the equivalent OS
> functions and
> > handle return errors.
> > DbgHelpLoader should never crash
> or assert;
> > missing functions should behave
> > like failing functions.
> >
> > 3) However, I added a one liner to
> the hs-err
> > file indicating the state of
> > the dbghelp dll - version info,
> what functions
> > were missing etc. This may
> > help understanding weird or
> missing callstacks.
> >
> > 4) I removed the implementation
> for shutdown
> > (WindowsDecoder::shutdown). I
> > think there is no valid reason to
> ever shutdown
> > the decoder. For one, we
> > may crash right at the end, and
> still it would be
> > nice to have callstacks.
> > And then, why spend cycles
> shutting down the
> > decoder when we could just let
> > it end with the process?
> >
> > 5) This code gets used during
> error reporting.
> > So no VM infrastructure must
> > be used to avoid circular crashes
> and VM
> > initialization dependencies. So,
> > to synchronize, this code uses raw
> windows
> > CriticalSection objects.
> >
> > --
> >
> > Next step will be revamping
> handling of the
> > Symbol APIs. This will involve
> > removing the WindowsDecoder class,
> which
> > introduces other errors and really
> > makes no sense if the underlying
> dbghelp layer
> > does its own synchronization.
> >
> >
> > Thanks for reviewing!
> >
> > Kind Regards, Thomas
> >
> >
> >
> >
> >
>
>
More information about the hotspot-runtime-dev
mailing list