RFR(s): 8186349: [windows] Centralize dbghelp handling code
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Wed Aug 23 14:12:31 UTC 2017
Hi Thomas,
nice cleanup, thanks.
You could mention in the change summary or in the bug
that you added printing the decoder state into the hs_err
file. This is fine, but by the description I did not
expect a functional change.
Some minor remarks:
decoder_windows.cpp:
Copyright needs to be updated. Please check other files.
Include decoder_windows.hpp not sorted alphabetically.
decoder_windows.hpp:
Superfluous 'private' specification. There are two of them
now. (I think usually there is one space before private/public,
but this wasn't the case before, either, in this file.)
os_windows_x86.cpp
Why don't you add the two includes to unwind_windows_x86.hpp
and windbghelp.hpp into the normal list at the proper alphabetic
position? If there is a good reason for this, please put this
into a comment.
decoder.cpp
Copyright wrong.
You guard the #include by _WINDOWS and the code by _WIN32.
This is inconsistent.
Best regards,
Goetz.
> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> bounces at openjdk.java.net] On Behalf Of Thomas Stüfe
> Sent: Dienstag, 22. August 2017 15:05
> To: Reingruber, Richard <richard.reingruber at sap.com>; Ioi Lam
> <ioi.lam at oracle.com>; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(s): 8186349: [windows] Centralize dbghelp handling code
>
> p.s.
>
> I built on x86 and x64. Ran gtests and part of the jtreg tests
> (hotspot/runtime/ErrorReporting) for both platforms.
>
> Two jtreg tests failed, but errors have nothing to do with my change - some
> java cross-module-access issue. Do these tests get run regularly?
>
> I also tried to build without precompiled headers to see if any includes
> were missing, but ran into other missing includes first, that seems to be
> rotted a bit.
>
> ..Thomas
>
> On Tue, Aug 22, 2017 at 2:48 PM, Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
>
> > Hi all,
> >
> > please see new Webrev:
> >
> > http://cr.openjdk.java.net/~stuefe/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>
> > 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> 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-bo
> >>> unces 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
> >>> 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
> >>> Webrev: http://cr.openjdk.java.net/~stuefe/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/2
> >>> 017-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