Pls review 6898160 (S)

[paul.hohensee]

6898160: Need serviceability support for new vm argument type 'uint64_t'.

Webrev here

http://cr.openjdk.java.net/~phh/6898160/webrev.01/

A new vm argument type, 'uint64_t', was added as part of 6887571.  The
jdk library
code in jdk/share/native/sun/management/Flag.c that fetches all the vm
product flags
throws an exception if it doesn't understand the type of an argument.  
The change
for 6887571 didn't include full serviceability support for uint64_t.

The fix is to add serviceability support for uint64_t.  In addition, if
a flag has a type
unknown to the jdk library, we now assert in debug builds and ignore it
in product
builds.  Thus, no jdk library change is necessary.

The failing test,

jdk/test/com/sun/management/HotSpotDiagnosticMXBean/GetDiagnosticOptions.java,

now passes.

Thanks,

Paul

[Tom Rodriguez]

Wouldn't it make more sense for the logic of converting a string into  
a proper argument to live over in Flag instead of being buried in  
attachListener?  It seems more likely that no one will forget that way  
and I think the result would be quite a bit shorter.  It looks ok as  
it too.

tom

On Nov 4, 2009, at 11:49 AM, Paul Hohensee wrote:

> 6898160: Need serviceability support for new vm argument type  
> 'uint64_t'.
>
> Webrev here
>
> http://cr.openjdk.java.net/~phh/6898160/webrev.01/
>
> A new vm argument type, 'uint64_t', was added as part of 6887571.  
> The jdk library
> code in jdk/share/native/sun/management/Flag.c that fetches all the  
> vm product flags
> throws an exception if it doesn't understand the type of an  
> argument.  The change
> for 6887571 didn't include full serviceability support for uint64_t.
>
> The fix is to add serviceability support for uint64_t.  In addition,  
> if a flag has a type
> unknown to the jdk library, we now assert in debug builds and ignore  
> it in product
> builds.  Thus, no jdk library change is necessary.
>
> The failing test,
>
> jdk/test/com/sun/management/HotSpotDiagnosticMXBean/
> GetDiagnosticOptions.java,
>
> now passes.
>
> Thanks,
>
> Paul
>

[mandy.chung]

Looks okay to me.  Minor nit:
   attachListener.cpp line 267: "unsigned integer" in the error message
should be "unsigned 64-bit value" or something like that.

Mandy

Paul Hohensee wrote:

> 6898160: Need serviceability support for new vm argument type 'uint64_t'.
>
> Webrev here
>
> http://cr.openjdk.java.net/~phh/6898160/webrev.01/
>
> A new vm argument type, 'uint64_t', was added as part of 6887571.  The
> jdk library
> code in jdk/share/native/sun/management/Flag.c that fetches all the vm
> product flags
> throws an exception if it doesn't understand the type of an argument.  
> The change
> for 6887571 didn't include full serviceability support for uint64_t.
>
> The fix is to add serviceability support for uint64_t.  In addition,
> if a flag has a type
> unknown to the jdk library, we now assert in debug builds and ignore
> it in product
> builds.  Thus, no jdk library change is necessary.
>
> The failing test,
>
> jdk/test/com/sun/management/HotSpotDiagnosticMXBean/GetDiagnosticOptions.java,
>
>
> now passes.
>
> Thanks,
>
> Paul
>

[paul.hohensee]

Thanks for the quick review.

I changed it to "flag value must be an unsigned 64-bit integer".

Paul

Mandy Chung wrote:

> Looks okay to me.  Minor nit:
>   attachListener.cpp line 267: "unsigned integer" in the error message
> should be "unsigned 64-bit value" or something like that.
>
> Mandy
>
> Paul Hohensee wrote:
>> 6898160: Need serviceability support for new vm argument type
>> 'uint64_t'.
>>
>> Webrev here
>>
>> http://cr.openjdk.java.net/~phh/6898160/webrev.01/
>>
>> A new vm argument type, 'uint64_t', was added as part of 6887571.  
>> The jdk library
>> code in jdk/share/native/sun/management/Flag.c that fetches all the
>> vm product flags
>> throws an exception if it doesn't understand the type of an
>> argument.  The change
>> for 6887571 didn't include full serviceability support for uint64_t.
>>
>> The fix is to add serviceability support for uint64_t.  In addition,
>> if a flag has a type
>> unknown to the jdk library, we now assert in debug builds and ignore
>> it in product
>> builds.  Thus, no jdk library change is necessary.
>>
>> The failing test,
>>
>> jdk/test/com/sun/management/HotSpotDiagnosticMXBean/GetDiagnosticOptions.java,
>>
>>
>> now passes.
>>
>> Thanks,
>>
>> Paul
>>
>

[Daniel D. Daugherty]

Paul Hohensee wrote:
> 6898160: Need serviceability support for new vm argument type 'uint64_t'.
>
> Webrev here
>
> http://cr.openjdk.java.net/~phh/6898160/webrev.01/

src/share/vm/runtime/globals.cpp
    No comments.


src/share/vm/services/attachListener.cpp
    No comments other than agreeing with Mandy about the message.
    Update: agree with the new message.

src/share/vm/services/management.cpp
    No comments.


Thumbs up!

Dan

[paul.hohensee]

Thanks, :)

Paul

Daniel D. Daugherty wrote:

> Paul Hohensee wrote:
>> 6898160: Need serviceability support for new vm argument type
>> 'uint64_t'.
>>
>> Webrev here
>>
>> http://cr.openjdk.java.net/~phh/6898160/webrev.01/
>
> src/share/vm/runtime/globals.cpp
>    No comments.
>
>
> src/share/vm/services/attachListener.cpp
>    No comments other than agreeing with Mandy about the message.
>    Update: agree with the new message.
>
> src/share/vm/services/management.cpp
>    No comments.
>
>
> Thumbs up!
>
> Dan
>