RFR(XS): 8212883: Setting a double manageable flag with jcmd/jinfo crashes the JVM (original) (raw)

Tony Printezis tprintezis at twitter.com
Wed Oct 24 12:53:11 UTC 2018

David,

See inline below.

————— Tony Printezis | @TonyPrintezis | tprintezis at twitter.com

On October 23, 2018 at 7:16:05 PM, David Holmes (david.holmes at oracle.com) wrote:

Hi Tony,

On 24/10/2018 8:13 AM, Tony Printezis wrote:

Webrev here:

http://cr.openjdk.java.net/~tonyp/8212883/webrev.0/ Currently, HotSpot doesn’t actually have any double manageable flags, which is why I think no-one has hit this before. I recently added a couple to our own builds and I noticed that setting them is not handled correctly in the VM. The fix is pretty trivial (mostly cut-and-paste from what the code does for the other types).

I agree the fix is pretty obvious and straight-forward.

Yeah, the only subtle difference is that in set_flag_from_jvalue() I get the double value from new_value.d (the double member of the union) instead of new_value.j (all int types seems to use the jlong). FWIW.

I tested it by introducing a dummy double manageable flag and I can set it

with jinfo/jcmd and jconsole (these cover all the various paths in the changes). Is it worth expanding the serviceability/attach/AttachSetGetFlag.java test to also get/set a double flag (I’d need to introduce a dummy double manageable flag to do that though)?

I hate to see new code untested ... but then it seems we don't have tests for all the existing types of flags anyway.

I would also like to have a test for this to avoid any regressions in the future. How about I push this change as is and create a new CR to expand the AttachSetGetFlag test to get/set flags of all types? I could just add a bunch of test flags only in debug builds for this purpose.

Reviewed.

Thanks!

Tony

Thanks, David

Regards,

Tony

————— Tony Printezis | @TonyPrintezis | tprintezis at twitter.com

More information about the hotspot-runtime-dev mailing list