Request for review (#4) 7174978: NPG: Fix bactrace builder for class redefinition

[David Holmes]

On 18/01/2013 12:06 AM, Coleen Phillimore wrote:
>
> On 01/17/2013 06:27 AM, David Holmes wrote:
>> On 17/01/2013 6:48 AM, Coleen Phillimore wrote:
>>> On 1/16/2013 2:19 AM, David Holmes wrote:
>>>>>> src/share/vm/oops/constantPool.hpp
>>>>>>
>>>>>> ! void add_version(int version) {
>>>>>> ! if (version == -1) {
>>>>>> ! _saved._version = version; // keep overflow
>>>>>> ! } else {
>>>>>> ! int new_version = version++;
>>>>>> ! _saved._version = version == INT_MAX ? -1 : version;
>>>>>> ! }
>>>>>> ! }
>>>>>>
>>>>>> new_version is unused here but I think you meant to do
>>>>>>
>>>>>> _saved._version = new_version == INT_MAX ? -1 : version;
>>>>>>
>>>>>
>>>>> This was a pre-code-review cleanup... I didn't mean to assign
>>>>> version++
>>>>> into new_version. The second line stays the same. I retested this fix.
>>>>
>>>> I'm unclear what the final form of this code now looks like.
>>>
>>> void add_version(int version) {
>>> if (version == -1) {
>>> _saved._version = version; // keep overflow
>>> } else {
>>> version++;
>>> _saved._version = (version == INT_MAX) ? -1 : version;
>>> }
>>> }
>>
>> But now you "overflow" too early - version==INT_MAX is not an
>> overflow. If you check the preincrement value against INT_MAX then you
>> have an overflow on the increment - hence I assume you intended
>>
>> int new_version = version++;
>> _saved._version = new_version == INT_MAX ? -1 : version;
>
> Actually, I want to save version++ in saved_version. I want to add one
> to the version passed in. The version of the constant pool passed in is
> from the old version of the constant pool.

Yes and that is what the code I had does - except that if you overflowed 
INT_MAX you stored -1, which seemed to be what you wanted.

> Should I rename this function increment_version(version)? That would
> probably be more clear. Maybe I should check for overflow by letting it
> go negative, like we do in the symbol.hpp _refcount.
>
> void increment_and_save_version(int version) {
> _saved._version = (version >=0) ? version++ : version; // keep
> overflowed value
> }

That saves the pre-incremented value. But otherwise it is a simpler 
formulation.

David


> Thanks,
> Coleen
>>
>> Thanks,
>> David
>