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

[Zhengyu Gu]

Hi Thomas,

Looks good overall.

symbolengine.cpp:

Is there reason to use ::malloc()/::free() instead of 
os::malloc()/os::free() ?


Line #602:
   buf might not be null-terminated if filename is longer than buffer len.


Thanks,

-Zhengyu







On 09/05/2017 09:05 AM, Thomas Stüfe wrote:
> 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> 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-
>>> 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
>>> 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
>>> Webrev:
>>> 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)
>>>
>>> -------------
>>>
>>> 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
>>