RFR (XS): 8003879: Duplicate definitions in vmStructs
Mikael Vidstedt
mikael.vidstedt at oracle.com
Thu Nov 22 14:19:27 PST 2012
On 2012-11-21 20:17, David Holmes wrote:
> Hi Mikael,
>
> On 22/11/2012 11:42 AM, Mikael Vidstedt wrote:
>>
>> Please review the below change.
>>
>> The change for 7045397 introduced a couple of duplicate entries in the
>> vmStructs::localHotSpotVMTypes array. This shows up when using the jmap
>> tool in a rather ugly way:
> <snip>
>
> Other than an indentation problem (which I don't think you introduced)
> this fixup seems fine.
For some reason that's the standard for declaring types in that long
list - aligning on the first letter of the type name. I followed suit.
>
>> In addition to removing the two duplicated entries I also added a
>> simple, naive runtime test to walk through and make sure no type is
>> repeated. The VMStructs::init only called in debug_only so there's no
>> startup overhead in product, but it may be better to turn the test into
>> a unit test and only running it as part of ExecuteInternalVMTests.
>> Feedback appreciated!
>
> I have a few comments on this part:
>
> - Array indices should be int's not size_t (ie signed not unsigned)
Will fix.
> - I can't clearly see how the localHotSpotVMTypes array is declared or
> filled but I assume there is guaranteed to be a sentinel entry at the
> end so that we don't index past the end?
Yeah. I'm not sure you want to know. It took me a few minutes to figure
it out. It turns out that the GENERATE_VM_TYPE_LAST_ENTRY macro which
generates the end marker is passed to VM_TYPES, VM_TYPES_CPU and
VM_TYPES_OS_CPU. But only the last one is allowed to actually allowed to
call the macro and actually generate the marker, meaning only the
"implementation" of VM_TYPES_OS_CPU actually has the call to
last_entry(). Not sure why the end marker isn't just explicitly added at
the end instead, but I think I'll queue that up for later.
All in all, as far as I can tell there is one and only one end marker
and it is generated as part of the expansion of VM_TYPES_OS_CPU.
> - assert(0, ...) should be assert(false, ...) (as per style guide [1]
> ;-) )
Will fix.
> That all said I'm not sure this test belongs there, but I don't feel
> strongly either way.
Me neither :)
Cheers,
Mikael
>
> Cheers,
> David
>
> [1] https://wikis.oracle.com/display/HotSpotInternals/StyleGuide
>
>>
>> http://cr.openjdk.java.net/~mikael/8003879/webrev.00/
>>
>> Cheers,
>> Mikael
>>
More information about the serviceability-dev
mailing list