RFR (XS): 8003879: Duplicate definitions in vmStructs

[Mikael Vidstedt]

On 2012-11-22 16:35, David Holmes wrote:
> Hi Mikael,
>
> I prefer this as an internal test too. Though I don't understand why 
> execute_internal_vm_tests is in jni.cpp rather than jvm.cpp - this is 
> an enhancement to the VM interface not the JNI interface isn't it? 
> (Separate issue of course).

I think we'll find that when we get a few more tests in there it's time 
to look at breaking it out into a separate file completely or do it in a 
completely different way, but that's for another day. I do hope it's 
soon though.

> BTW the weird indentation makes more sense viewing frames rather than 
> diffs :)

Right :)

>
> Looks good to go.

Thanks! Eagerly awaiting a second review!

/Mikael

>
> David
>
> On 23/11/2012 10:16 AM, Mikael Vidstedt wrote:
>>
>> I gave it some more thought and decided to try what it would look like
>> having the test be part of the ExecuteInternalVMTests family instead. I
>> like this one better personally, but comments are appreciated!
>>
>> http://cr.openjdk.java.net/~mikael/8003879/webrev.02
>>
>> Cheers,
>> Mikael
>>
>> On 2012-11-22 14:25, Mikael Vidstedt wrote:
>>>
>>> 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
>>>>>>
>>>>
>>>
>>