RFR(s): 8186349: [windows] Centralize dbghelp handling code
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Aug 24 09:15:04 UTC 2017
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> wrote:
> Looks perfect now, Reviewed.
>
> Best,
> Goetz.
>
> > -----Original Message-----
> > From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> > Sent: Donnerstag, 24. August 2017 10:51
> > To: Ioi Lam <ioi.lam at oracle.com>; Reingruber, Richard
> > <richard.reingruber at sap.com>; Lindenmaier, Goetz
> > <goetz.lindenmaier at sap.com>
> > Cc: 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-
> > handling/webrev.02/webrev/
> >
> >
> > Delta to last:
> > http://cr.openjdk.java.net/~stuefe/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> > 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-
> > centralize-dbghelp-handling/webrev.01/webrev/
> > <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> > 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> > 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-
> > runtime-dev-bounces at openjdk.java.net <mailto:hotspot-runtime-dev-
> > 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>
> > 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>
> > Webrev:
> > http://cr.openjdk.java.net/~stuefe/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-
> > runtime-dev/2017-August/024286.html
> > <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