Code review request: 7071311 Decoder enhancement

[David Holmes]

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?
2. Who calls Decoder::shutdown ?

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