RFR (XS): 8003879: Duplicate definitions in vmStructs

[David Holmes]

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).

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

Looks good to go.

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
>>>>>
>>>
>>
>