RFR: 8318026: jcmd should provide access to detailed JVM object information [v9]

[Andrew Dinn]

On Thu, 28 Mar 2024 17:15:26 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

>>> In my opinion, UnlockDiagnosticVMOptions is not a good enough safeguard since it guards a whole swathe of switches that we may instruct the customer to enable. Once enabled, my experience is that UnlockDiagnosticVMOptions often lingers around. It is not unusual for customer scenarios to have set +UnlockDiagnosticVMOptions because of some years ago support cases.
>> 
>> I think we also need to consider the flip side of this argument. Is this something that some customers might want to always enable, but don't want to always have UnlockDiagnosticVMOptions enabled. A new command line flag would be needed in that case.
>> 
>> Also, isn't UnlockDiagnosticVMOptions meant for enabling the use of diagnostic command line flags? Do we have examples of it enabling a hotspot feature that does not also require setting a diagnostic command line flag?
>
>> I think we also need to consider the flip side of this argument. Is this something that some customers might want to always enable, but don't want to always have UnlockDiagnosticVMOptions enabled. A new command line flag would be needed in that case.
>> 
>> Also, isn't UnlockDiagnosticVMOptions meant for enabling the use of diagnostic command line flags? Do we have examples of it enabling a hotspot feature that does not also require setting a diagnostic command line flag?
> 
> Thanks Chris - that is correct now I check the wording, UnlockDiagnosticVMOptions: "Enable normal processing of flags relating to field diagnostics"
> 
> Yes it makes flags which are DIAGNOSTIC available at the command-line.  We have UnlockExperimentalVMOptions also.
> 
> My original reason for the guard, is that the risk of fooling  the "good oop" check is that we could possibly crash (so that's answering your other question).  Inspecting an arbitrary pointer that looks enough like a Java heap object to fool the test, means it might try and resolve bad addresses for Klass pointer or field data.
> 
> I also have a stress test which examines every address in a Java heap to test for crashes.  That's obviously long-running as it does a jcmd for every iteration.  But I can't currently get a crash, trying G1, ZGC, ZGCGenerational.
> 
> Having a global guard to prevent usage of VM.inspect may be needed less than I thought, but will wait on the other thread before saying that is really the case.  We could have a new -XX flag to guard it (whether specific to this command or a more general UnlockDiagnosticFeatures.  I would definitely stick to this being in all builds, as we frequently "debug" non-debug builds.
> 
> Also, for your comment above about the new version of dbg_is_good_oop: this was added because although there are not many callers of it, I did not want to upset them.  During my earlier testing the detailed version of this method did upset something, although I cannot reproduce that today.  
> 
> I will rerun some tests on these items...

@kevinjwalls Thanks for clarifying the relevant behaviours and code paths as well as identifying a few places where documentation might help devs avoid tripping over any issues. Much appreciated.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-2031966028