RFR (XS): 8003879: Duplicate definitions in vmStructs

[Mikael Vidstedt]

New webrev available here:

http://cr.openjdk.java.net/~mikael/8003879/webrev.01

Cheers,
Mikael

On 2012-11-22 14:19, Mikael Vidstedt wrote:
> 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
>>>
>