Request for review: 7046490 - Preallocated OOME objects should obey Throwable stack trace protocol

[David Holmes]

Tom Rodriguez said the following on 07/21/11 02:40:
> On Jul 20, 2011, at 2:26 AM, David Holmes wrote:
> 
>> Hi Tom,
>>
>> Thomas Rodriguez said the following on 07/20/11 00:09:
>>> You can use ik->java_mirror()->obj_field(offset) to read static fields. I don't know why the others are done as they are. maybe just history. 
>> Is there a different way to calculate the offset in this case? I tried this using the existing static_unassigned_stacktrace_offset and while I had no problems reading that value and setting it into the Throwable, as soon as I tried to access the stacktrace field containing that value I got a SEGV in LinkResolver::resolve_invokevirtual
> 
> Sorry, my suggestion uses real offsets but static_field_addr uses an offset from the base of the static field area and I didn't notice that.  You really should be looking up the offset using compute_offset instead of hardcoding it though.  That will return the real offset needed by obj_field.  If you hardcode it then adding static fields to Throwable could create a mismatch between the real layout and the JVM.  The Reference static fields have the same problem though I guess we've never cared about the potential mismatch issues.

So Throwable is one of the classes that seems to have to use hard-coded 
offsets - see compute_hard_coded_offsets(). It may be that if I don't 
try to compute the offset until I actually need it then it might be 
okay, but this is moving from a trivial fix to something more elaborate. 
  So I'm going to stick with my original fix if that is ok. We can file 
a RFE to update these old-style static offset calculations to the new 
scheme.

>> I must admit I was a bit puzzled by your suggestion. The java_mirror() is the oop for the Class object, but I didn't think that we stored static fields as instance fields of the Class object ???
> 
> That was done as part of 7017732.  ik->static_field_addr is doing this behind your back:

Thanks for the info (and thanks to to Krystal). Hard to keep track of 
everything these days :)

Cheers,
David
-----

> address instanceKlass::static_field_addr(int offset) {
>   return (address)(offset + instanceMirrorKlass::offset_of_static_fields() + (intptr_t)java_mirror());
> }
> 
> I really should have fixed these uses instead of papering over them.
> 
> tom
> 
>> Thanks,
>> David
>>
>>> tom
>>> On Jul 18, 2011, at 9:02 PM, David Holmes <David.Holmes at oracle.com> wrote:
>>>> Paul Hohensee said the following on 07/19/11 13:32:
>>>>> Looks good, except, could you use obj_field() in unassigned_stacktrace() instead of checking UseCompressedOops there?
>>>> Thanks Paul.
>>>>
>>>> I don't think I can use obj_field() as it is for accessing instances and this is a static field. I copied the code for reading the static field from the java.lang.ref functions:
>>>>
>>>> 2286 // Support for java_lang_ref_Reference
>>>> 2287 oop java_lang_ref_Reference::pending_list_lock() {
>>>> 2288   instanceKlass* ik = instanceKlass::cast(SystemDictionary::Reference_klass());
>>>> 2289   address addr = ik->static_field_addr(static_lock_offset);
>>>> 2290   if (UseCompressedOops) {
>>>> 2291     return oopDesc::load_decode_heap_oop((narrowOop *)addr);
>>>> 2292   } else {
>>>> 2293     return oopDesc::load_decode_heap_oop((oop*)addr);
>>>> 2294   }
>>>> 2295 }
>>>> 2296
>>>> 2297 HeapWord *java_lang_ref_Reference::pending_list_addr() {
>>>> 2298   instanceKlass* ik = instanceKlass::cast(SystemDictionary::Reference_klass());
>>>> 2299   address addr = ik->static_field_addr(static_pending_offset);
>>>> 2300   // XXX This might not be HeapWord aligned, almost rather be char *.
>>>> 2301   return (HeapWord*)addr;
>>>> 2302 }
>>>> 2303
>>>> 2304 oop java_lang_ref_Reference::pending_list() {
>>>> 2305   char *addr = (char *)pending_list_addr();
>>>> 2306   if (UseCompressedOops) {
>>>> 2307     return oopDesc::load_decode_heap_oop((narrowOop *)addr);
>>>> 2308   } else {
>>>> 2309     return oopDesc::load_decode_heap_oop((oop*)addr);
>>>> 2310   }
>>>> 2311 }
>>>>
>>>> David
>>>> -----
>>>>
>>>>> Paul
>>>>> On 7/7/11 1:25 AM, David Holmes wrote:
>>>>>> http://cr.openjdk.java.net/~dholmes/7046490/webrev/
>>>>>>
>>>>>> This is a simple fix in the VM to honour the Throwable immutability protocol that was defined for Java 7. Pre-allocated Throwable instances are normally immutable, but that is not the case for a group of pre-allocated OutOfMemoryError instances. To conform to the protocol, when we set the backtrace (VM representation of the stackTrace) we have to set the stackTrace field to the sentinel value stored in the static Throwable.UNASSIGNED_STACK field.
>>>>>>
>>>>>> With this fix in place the workaround in Throwable.java, under 7045138, can be backed out.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>
>>>>>>
>