Code review request: 7071311 Decoder enhancement

[Zhengyu Gu]

Hi David,

Thanks for reviewing.

On 10/20/2011 5:01 AM, David Holmes wrote:
> Hi Zhengyu,
>
> This isn't a full review as I don't know the details of either the 
> windows decoder or the Elf decoder. Overall the thread-safety changes 
> seem reasonable: the static Decoder methods encapsulate acquiring the 
> current _decoder instance under protection of a lock. Two things I'm 
> unclear on though:
>
> 1. Which threads might concurrently try to use a Decoder?
Native memory tracking will use Decoder for callsite symbols.

>
> 2. Who calls Decoder::shutdown ?
>
None right now, eventually will be called by native memory tracking code 
under low memory scenario. The idea is to allow native memory tracking 
code to free as much memory as possible, so it can proceed a final dump 
to its client, when native out-of-memory is encountered, even means 
there won't have decoded symbols.

Regarding Mach-O, my understanding is that OSX binaries use Mach-O 
format 
(http://developer.apple.com/library/mac/#documentation/DeveloperTools/Conceptual/MachOTopics/1-Articles/executing_files.html 
), BSD still uses Elf(?)

I will fix the rest.

Thanks,

-Zhengyu

> A few minor comments:
>
> src/os/windows/vm/decoder_windows.cpp
>
> *** 1,7 ****
>   /*
> !  * Copyright (c) 1997, 2010, Oracle and/or its affiliates. All 
> rights reserved.
> --- 1,7 ----
>   /*
> !  * Copyright (c) 2010, Oracle and/or its affiliates. All rights 
> reserved.
>
> This change to the copyright can't be correct. Also some new files 
> have a 2010 copyright instead of 2011.
>
>
> -----
>
> src/os/windows/vm/decoder_windows.cpp
>
>       if (_pfnSymGetSymFromAddr64(::GetCurrentProcess(), 
> (DWORD64)addr, &displacement, pSymbol)) {
>         if (buf != NULL) {
> !         if (_demangle(pSymbol->Name, buf, buflen)) {
>             jio_snprintf(buf, buflen, "%s", pSymbol->Name);
>           }
>         }
> !       if(offset) *offset = (int)displacement;
> !       return true;
>       }
>     }
> !   if (buf != 0 && buflen > 0) buf[0] = '\0';
> !   if (offset) *offset = -1;
> !   return false;
>   }
>
> Stylistically I think we prefer to maintain explicit tests against 
> NULL, and we should be consistent. In the above there is:
>  if (buf != NULL)
>  if (offset)
>  if (buf != 0 ...)
> these should all be the same form and that should be an explicit test 
> against NULL. Semantically they are equivalent of course (provided 
> NULL hasn't been given some weird definition).
>
> ---
>
> src/share/vm/utilities/decoder.cpp
>
>  33   #include "decoder_machO.hpp"
>
> machO? Not sure if that is meant to be a pun/joke :) but wouldn't OSX 
> be more consistent with other OSX specific features? But I'm also 
> unclear whether this is really meant to be BSD or OSX as I assume they 
> are not the same.
>
>  52   // Decoder is a secondary service. Althrough, it is good to have,
>
> typo: Althrough -> Although
>
>
> Cheers,
> David
> -----
>
>
> On 20/10/2011 2:59 AM, Zhengyu Gu wrote:
>> Hi,
>>
>> This is a refactoring and enhancement of current decoder. The primary
>> goal is to make it thread-safe, so it can be used by other than just
>> error handler. It also addressed CR 7067266
>> (http://monaco.sfbay.sun.com/detail.jsf?cr=7067266).
>>
>> CR 7071311: http://monaco.sfbay.sun.com/detail.jsf?cr=7071311
>> Webrev: http://cr.openjdk.java.net/~zgu/7071311/webrev.00/
>>
>> The changes have been tested on Win32, Linux 32 and Solaris.
>>
>> Thanks,
>>
>> -Zhengyu