RFR (S): 8004747: Remove last_entry from VM_STRUCT macros

[Mikael Vidstedt]

Serguei, Yumin - thanks for the reviews! I will update the test in the 
assert to mention the name of the array in question!

Cheers,
Mikael

On 12/31/2012 12:30 PM, Yumin Qi wrote:
> Looks good. Thanks for the fix, the macros is quite complex to 
> understand. Agree with Serguei the assertion with better description.
>
> Thanks
> Yumin
>
> On 12/31/2012 10:36 AM, serguei.spitsyn at oracle.com wrote:
>> Mikael,
>>
>> It looks good.
>> It is also a nice simplification!
>>
>> The only thing I'd suggest is to be more specific in the assert at 
>> the end of the vmStructs.cpp.
>> Instead of "Incorrect last entry" it'd be better to have "Incorrect 
>> struct last entry", etc.
>>
>> Also, it'd be useful if Yumin had a chance to surfacely look at the fix.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 12/28/12 5:17 PM, Mikael Vidstedt wrote:
>>>
>>> Thanks David!
>>>
>>> Still need a 2nd reviewer!
>>>
>>> Cheers,
>>> Mikael
>>>
>>> On 12/28/2012 3:34 PM, David Holmes wrote:
>>>> Mikael,
>>>>
>>>> That all sounds fine to me.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 29/12/2012 3:48 AM, Mikael Vidstedt wrote:
>>>>>
>>>>> David,
>>>>>
>>>>> I did two things (both on linux/x86_64 only):
>>>>>
>>>>> 1. I compared the pre-processor output manually. Unfortunately 
>>>>> there are
>>>>> line numbers sprinkled around in the pre-processed file which makes
>>>>> comparing the output a bit more complex than it should be. Modulo the
>>>>> line numbers and some white space changes the outputs matche.
>>>>> 2. I wrote a small test which mimics what the SA does - it looks 
>>>>> up the
>>>>> four exported structures dynamically and reads the data. Apart 
>>>>> from the
>>>>> "address" field in the VMStructEntry entries the outputs almost [1]
>>>>> match. The address field is expected to vary since it reflects the
>>>>> actual placement in memory of various static variables.
>>>>>
>>>>> Are there other experiments you can think of?
>>>>>
>>>>> Cheers,
>>>>> Mikael
>>>>>
>>>>> [1] Using this tool I actually found one bug in the table - one entry
>>>>> (ClassLoaderDataGraph::_unloading) is declared as nonstatic when 
>>>>> it is
>>>>> indeed static. I'll file a separate bug for that.
>>>>>
>>>>> On 12/11/2012 8:16 PM, David Holmes wrote:
>>>>>> Hi Mikael,
>>>>>>
>>>>>> This looks okay to me - I like the simplification. However as I 
>>>>>> don't
>>>>>> perform mental macro substitution particularly well (okay, not at 
>>>>>> all
>>>>>> ;-)) I was wondering if there was a simple validation test where we
>>>>>> could compare the pre-processed output before and after?
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>> On 11/12/2012 7:01 AM, Mikael Vidstedt wrote:
>>>>>>>
>>>>>>> Please review the following change:
>>>>>>>
>>>>>>> Webrev: http://cr.openjdk.java.net/~mikael/8004747/webrev.00/
>>>>>>>
>>>>>>> Background:
>>>>>>>
>>>>>>> This patch does 3 things (depending on how you count it):
>>>>>>>
>>>>>>> 1. Move the call to generating the last entry for the VM structures
>>>>>>> database
>>>>>>>
>>>>>>> The vmStructs functionality is a macro based way of creating a 
>>>>>>> database
>>>>>>> of information about VM internal structures, types and constants.
>>>>>>>
>>>>>>> The database for structures is defined in runtime/vmStructs.cpp 
>>>>>>> in an
>>>>>>> array called localHotSpotVMStructs. The content of the array is
>>>>>>> generated with calls to a few different macros (VM_STRUCTS,
>>>>>>> VM_STRUCTS_PARALLELGC, VM_STRUCTS_CMS, VM_STRUCTS_G1, 
>>>>>>> VM_STRUCTS_CPU and
>>>>>>> VM_STRUCTS_OS_CPU respectively). Common for all these macros is 
>>>>>>> the fact
>>>>>>> that the last argument passed in is another macro
>>>>>>> (GENERATE_VM_STRUCT_LAST_ENTRY) which is a generator for the end
>>>>>>> marker/last entry in the database; a special entry which can be 
>>>>>>> easily
>>>>>>> located by an external consumer of the information.
>>>>>>>
>>>>>>> Even though the end marker generator macro
>>>>>>> (GENERATE_VM_STRUCT_LAST_ENTRY) is passed to all the VM_STRUCT* 
>>>>>>> macros,
>>>>>>> the actual end marker must be generated once and only once. The 
>>>>>>> way this
>>>>>>> is currently handled is that only the last VM_STRUCT macro, which
>>>>>>> happens to be VM_STRUCTS_OS_CPU, actually makes use of its last 
>>>>>>> argument
>>>>>>> and adds it to the end of the array.
>>>>>>>
>>>>>>> Not only is this fairly complex to understand, it's also more 
>>>>>>> fragile
>>>>>>> than it needs to be.
>>>>>>>
>>>>>>> This change removes the last argument for all the VM_STRUCT 
>>>>>>> macros and
>>>>>>> instead explicitly inserts the end marker at the end of the
>>>>>>> localHotSpotVMStructs array.
>>>>>>>
>>>>>>> The same VM_STRUCTS macros are being used in VMStructs::init to 
>>>>>>> do type
>>>>>>> checking. Instead of passing the end marker generator into the 
>>>>>>> macros
>>>>>>> the last parameter is instead CHECK_SENTINEL, which is defined 
>>>>>>> to expand
>>>>>>> to nothing, which means it can be safely removed.
>>>>>>>
>>>>>>> 2. Move the end marker generating for the VM types, VM int 
>>>>>>> constants and
>>>>>>> VM long constants databases
>>>>>>>
>>>>>>> Repeat the exact above story, but replace anything called 
>>>>>>> Structs/STRUCT
>>>>>>> with Types/TYPES, IntConstants/INT_CONSTANTS and
>>>>>>> LongConstants/LONG_CONSTANTS respectively.
>>>>>>>
>>>>>>> 3. Minor prettification
>>>>>>>
>>>>>>> In addition to the above the change also removes some superfluous
>>>>>>> backslashes from a few of the VM_STRUCT* macro calls.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Mikael
>>>>>>>
>>>>>
>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20130108/1f3d8176/attachment-0001.html