RFR: 8189685: need PerfMemory class update and a volatile_static_field support in VMStructs [v3]

Chris Plummer cjplummer at openjdk.org
Wed Aug 23 23:17:28 UTC 2023


On Wed, 23 Aug 2023 22:40:23 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> This change wasn't in the original patch, but the `GENERATE_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY` change above it was.  I found it when I noticed the use of `GENERATE_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY` and thought it should be renamed too. I was also doing "static volatile" -> "volatile stack" at the time so that is also part of this macros rename. 
>> 
>> As David points out, the purpose is to make it more general purpose, although for this macro only the name change is needed (`GENERATE_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY also has a minor implementation change that was needed).
>> 
>> The comments for both of these macros references "static pointer volatile" entries. That should changed to "volatile static". The type no longer needs to be a pointer type. 
>> 
>> I'm not certain what this CHECK macro does, but it was pre-existing, and there is also the `CHECK_STATIC_VM_STRUCT_ENTRY` macro above it which is identical except of the absence of "volatile" in the name and implementation. I think it verifies that the type  specified in the vmstructs definition of the field is actually the same as the field itself.  The macro will generate a compiler error if a cast was needed. I think this type of check can only be done with static fields. You would need an instance of the class/struct to do a similar check on an instance field. Thus CHECK_NONSTATIC_VM_STRUCT_ENTRY is much more complex and seems to be a runtime check that will assert if it fails.
>
> I can't convince myself that `volatile` is in the right place in `type volatile * dummy`. But `volatile * dummy` means that the pointer is volatile as opposed to the type being pointed to, which would match with the original macro name's use of `PTR_VOLATILE`.

But in that case the use of the macro doesn't make much sense. Regardless of the field type, vmstructs doesn't deal with volatile pointers to fields. It deals with volatile fields (that can be pointers or scalar). I think if anything the macro has always been wrong and should contain `volatile type * dummy`. It probably never fails because it is taking a non-volatile type and assigning it to a volatile type, which always works if the types are otherwise the same.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15373#discussion_r1303630417


More information about the serviceability-dev mailing list