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

[Yumin Qi]

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/20121231/2f4d8c79/attachment.html