Re: Request for review:8023477: Invalid CP index when reading ConstantPool

2013-08-26 Thread Staffan Larsen 
Looks good.

Thanks,
/Staffan

On 23 aug 2013, at 23:49, Jiangli Zhou jiangli.z...@oracle.com wrote:

 Hi,
 
 Please review the fix for 8023477:
 
  http://cr.openjdk.java.net/~jiangli/8023477/webrev.00/
 
 Both tests reported by the bug failed due to the same problem. They both are 
 passing now.
 
 The original memory reduction change for 8021948 turned out to be a little 
 trickier than I expected.
 
 Thanks,
 Jiangli
 
 
 On 08/23/2013 11:00 AM, Jiangli Zhou wrote:
 I found the issue in the SA code. For class without generic signature, the 
 InstanceKlass::_generic_signature_index is 0. Need to check for this case in 
 the SA code. I'm working on the fix and doing testing.
 
 Thanks,
 Jiangli
 
 On 08/23/2013 08:21 AM, Jiangli Zhou wrote:
 Hi Staffan,
 
 Thanks for the info. Will look into that one.
 
 Jiangli
 
 On 08/23/2013 05:27 AM, Staffan Larsen wrote:
 Unfortunately there are two more tests that started failing after the 
 initial change. See: JDK-8023477.
 
 /Staffan
 
 On 23 aug 2013, at 01:16, Jiangli Zhou jiangli.z...@oracle.com wrote:
 
 Hi Coleen,
 
 Yes, that's the case. Thanks for the review. I'll push this to hotspot-rt.
 
 Thanks,
 Jiangli
 
 On 08/22/2013 03:34 PM, Coleen Phillimore wrote:
 Hi,
 Is it the case that the old index isn't in the index map because it 
 didn't change?  If so, this looks correct.
 Thanks,
 Coleen
 
 
 On 08/22/2013 06:15 PM, Jiangli Zhou wrote:
 On 08/22/2013 03:10 PM, serguei.spit...@oracle.com wrote:
 Hi Jiangli,
 
 The fix is good and safe.
 I'm happy you fixed another case as well!
 Let's consider current bug as a clean-up issue so that we do not need 
 to file a separate bug. :)
 Ok.
 
 Thanks,
 Jiangli
 
 Thanks,
 Serguei
 
 
 On 8/22/13 2:50 PM, Jiangli Zhou wrote:
 Hi Serguei,
 
 I've also made change to the case that you discovered. Please let me 
 know if you think a separate bug should be filed to track it instead.
 
 http://cr.openjdk.java.net/~jiangli/8023547/webrev.01/
 
 Thanks,
 Jiangli
 
 On 08/22/2013 02:24 PM, Jiangli Zhou wrote:
 Hi Serguei,
 
 Thank you very much for the review and confirmation with the test.
 
 Jiangli
 
 On 08/22/2013 02:18 PM, serguei.spit...@oracle.com wrote:
 Hi Jiangli,
 
 Thank you for the quick fix which looks fine to me.
 I confirm the test is passed with it.
 
 Staffan, thank you for the regression isolation.
 I've noticed the following fragment in this file which seems has a 
 similar issue:
 
  // We also need to rewrite the parameter name indexes, if there is
  // method parameter data present
  if(method-has_method_parameters()) {
const int len = method-method_parameters_length();
MethodParametersElement* elem = 
 method-method_parameters_start();
 
for (int i = 0; i  len; i++) {
  const u2 cp_index = elem[i].name_cp_index;
  elem[i].name_cp_index = find_new_index(cp_index);
}
  }
 } // end rewrite_cp_refs_in_method()
 
 The result of the find_new_index() above is not checked for 0.
 
 Thanks,
 Serguei
 
 On 8/22/13 12:38 PM, Jiangli Zhou wrote:
 Hi Staffan, Serguei and others,
 
 Here is the webrev for the 8023547 fix:
 
 http://cr.openjdk.java.net/~jiangli/8023547/webrev.00/
 
 Thanks!
 
 Jiangli

Re: Request for review:8023477: Invalid CP index when reading ConstantPool

2013-08-26 Thread Jiangli Zhou 

Thanks, Staffan!

Jiangli

On 08/26/2013 01:13 AM, Staffan Larsen wrote:

Looks good.

Thanks,
/Staffan

On 23 aug 2013, at 23:49, Jiangli Zhou jiangli.z...@oracle.com wrote:


Hi,

Please review the fix for 8023477:

  http://cr.openjdk.java.net/~jiangli/8023477/webrev.00/

Both tests reported by the bug failed due to the same problem. They both are 
passing now.

The original memory reduction change for 8021948 turned out to be a little 
trickier than I expected.

Thanks,
Jiangli


On 08/23/2013 11:00 AM, Jiangli Zhou wrote:

I found the issue in the SA code. For class without generic signature, the 
InstanceKlass::_generic_signature_index is 0. Need to check for this case in 
the SA code. I'm working on the fix and doing testing.

Thanks,
Jiangli

On 08/23/2013 08:21 AM, Jiangli Zhou wrote:

Hi Staffan,

Thanks for the info. Will look into that one.

Jiangli

On 08/23/2013 05:27 AM, Staffan Larsen wrote:

Unfortunately there are two more tests that started failing after the initial 
change. See: JDK-8023477.

/Staffan

On 23 aug 2013, at 01:16, Jiangli Zhou jiangli.z...@oracle.com wrote:


Hi Coleen,

Yes, that's the case. Thanks for the review. I'll push this to hotspot-rt.

Thanks,
Jiangli

On 08/22/2013 03:34 PM, Coleen Phillimore wrote:

Hi,
Is it the case that the old index isn't in the index map because it didn't 
change?  If so, this looks correct.
Thanks,
Coleen


On 08/22/2013 06:15 PM, Jiangli Zhou wrote:

On 08/22/2013 03:10 PM, serguei.spit...@oracle.com wrote:

Hi Jiangli,

The fix is good and safe.
I'm happy you fixed another case as well!
Let's consider current bug as a clean-up issue so that we do not need to file a 
separate bug. :)

Ok.

Thanks,
Jiangli


Thanks,
Serguei


On 8/22/13 2:50 PM, Jiangli Zhou wrote:

Hi Serguei,

I've also made change to the case that you discovered. Please let me know if 
you think a separate bug should be filed to track it instead.

http://cr.openjdk.java.net/~jiangli/8023547/webrev.01/

Thanks,
Jiangli

On 08/22/2013 02:24 PM, Jiangli Zhou wrote:

Hi Serguei,

Thank you very much for the review and confirmation with the test.

Jiangli

On 08/22/2013 02:18 PM, serguei.spit...@oracle.com wrote:

Hi Jiangli,

Thank you for the quick fix which looks fine to me.
I confirm the test is passed with it.

Staffan, thank you for the regression isolation.
I've noticed the following fragment in this file which seems has a similar 
issue:

  // We also need to rewrite the parameter name indexes, if there is
  // method parameter data present
  if(method-has_method_parameters()) {
const int len = method-method_parameters_length();
MethodParametersElement* elem = method-method_parameters_start();

for (int i = 0; i  len; i++) {
  const u2 cp_index = elem[i].name_cp_index;
  elem[i].name_cp_index = find_new_index(cp_index);
}
  }
} // end rewrite_cp_refs_in_method()

The result of the find_new_index() above is not checked for 0.

Thanks,
Serguei

On 8/22/13 12:38 PM, Jiangli Zhou wrote:

Hi Staffan, Serguei and others,

Here is the webrev for the 8023547 fix:

http://cr.openjdk.java.net/~jiangli/8023547/webrev.00/

Thanks!

Jiangli

Re: Request for review:8023477: Invalid CP index when reading ConstantPool

2013-08-23 Thread serguei.spit...@oracle.com 

Hi Jiangli,

The fix looks good and safe.

Thanks,
Serguei

On 8/23/13 2:49 PM, Jiangli Zhou wrote:

Hi,

Please review the fix for 8023477:

  http://cr.openjdk.java.net/~jiangli/8023477/webrev.00/

Both tests reported by the bug failed due to the same problem. They 
both are passing now.


The original memory reduction change for 8021948 turned out to be a 
little trickier than I expected.


Thanks,
Jiangli


On 08/23/2013 11:00 AM, Jiangli Zhou wrote:
I found the issue in the SA code. For class without generic 
signature, the InstanceKlass::_generic_signature_index is 0. Need to 
check for this case in the SA code. I'm working on the fix and doing 
testing.


Thanks,
Jiangli

On 08/23/2013 08:21 AM, Jiangli Zhou wrote:

Hi Staffan,

Thanks for the info. Will look into that one.

Jiangli

On 08/23/2013 05:27 AM, Staffan Larsen wrote:
Unfortunately there are two more tests that started failing after 
the initial change. See: JDK-8023477.


/Staffan

On 23 aug 2013, at 01:16, Jiangli Zhou jiangli.z...@oracle.com 
wrote:



Hi Coleen,

Yes, that's the case. Thanks for the review. I'll push this to 
hotspot-rt.


Thanks,
Jiangli

On 08/22/2013 03:34 PM, Coleen Phillimore wrote:

Hi,
Is it the case that the old index isn't in the index map because 
it didn't change?  If so, this looks correct.

Thanks,
Coleen


On 08/22/2013 06:15 PM, Jiangli Zhou wrote:

On 08/22/2013 03:10 PM, serguei.spit...@oracle.com wrote:

Hi Jiangli,

The fix is good and safe.
I'm happy you fixed another case as well!
Let's consider current bug as a clean-up issue so that we do 
not need to file a separate bug. :)

Ok.

Thanks,
Jiangli


Thanks,
Serguei


On 8/22/13 2:50 PM, Jiangli Zhou wrote:

Hi Serguei,

I've also made change to the case that you discovered. Please 
let me know if you think a separate bug should be filed to 
track it instead.


http://cr.openjdk.java.net/~jiangli/8023547/webrev.01/

Thanks,
Jiangli

On 08/22/2013 02:24 PM, Jiangli Zhou wrote:

Hi Serguei,

Thank you very much for the review and confirmation with the 
test.


Jiangli

On 08/22/2013 02:18 PM, serguei.spit...@oracle.com wrote:

Hi Jiangli,

Thank you for the quick fix which looks fine to me.
I confirm the test is passed with it.

Staffan, thank you for the regression isolation.
I've noticed the following fragment in this file which seems 
has a similar issue:


  // We also need to rewrite the parameter name indexes, if 
there is

  // method parameter data present
  if(method-has_method_parameters()) {
const int len = method-method_parameters_length();
MethodParametersElement* elem = 
method-method_parameters_start();


for (int i = 0; i  len; i++) {
  const u2 cp_index = elem[i].name_cp_index;
  elem[i].name_cp_index = find_new_index(cp_index);
}
  }
} // end rewrite_cp_refs_in_method()

The result of the find_new_index() above is not checked for 0.

Thanks,
Serguei

On 8/22/13 12:38 PM, Jiangli Zhou wrote:

Hi Staffan, Serguei and others,

Here is the webrev for the 8023547 fix:

http://cr.openjdk.java.net/~jiangli/8023547/webrev.00/

Thanks!

Jiangli

Re: Request for review:8023477: Invalid CP index when reading ConstantPool

2013-08-23 Thread Jiangli Zhou 

Thanks again, Serguei!

Jiangli

On 08/23/2013 03:27 PM, serguei.spit...@oracle.com wrote:

Hi Jiangli,

The fix looks good and safe.

Thanks,
Serguei

On 8/23/13 2:49 PM, Jiangli Zhou wrote:

Hi,

Please review the fix for 8023477:

  http://cr.openjdk.java.net/~jiangli/8023477/webrev.00/

Both tests reported by the bug failed due to the same problem. They 
both are passing now.


The original memory reduction change for 8021948 turned out to be a 
little trickier than I expected.


Thanks,
Jiangli


On 08/23/2013 11:00 AM, Jiangli Zhou wrote:
I found the issue in the SA code. For class without generic 
signature, the InstanceKlass::_generic_signature_index is 0. Need to 
check for this case in the SA code. I'm working on the fix and doing 
testing.


Thanks,
Jiangli

On 08/23/2013 08:21 AM, Jiangli Zhou wrote:

Hi Staffan,

Thanks for the info. Will look into that one.

Jiangli

On 08/23/2013 05:27 AM, Staffan Larsen wrote:
Unfortunately there are two more tests that started failing after 
the initial change. See: JDK-8023477.


/Staffan

On 23 aug 2013, at 01:16, Jiangli Zhou jiangli.z...@oracle.com 
wrote:



Hi Coleen,

Yes, that's the case. Thanks for the review. I'll push this to 
hotspot-rt.


Thanks,
Jiangli

On 08/22/2013 03:34 PM, Coleen Phillimore wrote:

Hi,
Is it the case that the old index isn't in the index map because 
it didn't change?  If so, this looks correct.

Thanks,
Coleen


On 08/22/2013 06:15 PM, Jiangli Zhou wrote:

On 08/22/2013 03:10 PM, serguei.spit...@oracle.com wrote:

Hi Jiangli,

The fix is good and safe.
I'm happy you fixed another case as well!
Let's consider current bug as a clean-up issue so that we do 
not need to file a separate bug. :)

Ok.

Thanks,
Jiangli


Thanks,
Serguei


On 8/22/13 2:50 PM, Jiangli Zhou wrote:

Hi Serguei,

I've also made change to the case that you discovered. Please 
let me know if you think a separate bug should be filed to 
track it instead.


http://cr.openjdk.java.net/~jiangli/8023547/webrev.01/

Thanks,
Jiangli

On 08/22/2013 02:24 PM, Jiangli Zhou wrote:

Hi Serguei,

Thank you very much for the review and confirmation with the 
test.


Jiangli

On 08/22/2013 02:18 PM, serguei.spit...@oracle.com wrote:

Hi Jiangli,

Thank you for the quick fix which looks fine to me.
I confirm the test is passed with it.

Staffan, thank you for the regression isolation.
I've noticed the following fragment in this file which 
seems has a similar issue:


  // We also need to rewrite the parameter name indexes, if 
there is

  // method parameter data present
  if(method-has_method_parameters()) {
const int len = method-method_parameters_length();
MethodParametersElement* elem = 
method-method_parameters_start();


for (int i = 0; i  len; i++) {
  const u2 cp_index = elem[i].name_cp_index;
  elem[i].name_cp_index = find_new_index(cp_index);
}
  }
} // end rewrite_cp_refs_in_method()

The result of the find_new_index() above is not checked for 0.

Thanks,
Serguei

On 8/22/13 12:38 PM, Jiangli Zhou wrote:

Hi Staffan, Serguei and others,

Here is the webrev for the 8023547 fix:

http://cr.openjdk.java.net/~jiangli/8023547/webrev.00/

Thanks!

Jiangli