RFR(xs): 8159019: ResourceMark in ClassLoader::open_versioned_entry() is being used incorrectly

[Ioi Lam]

On 6/8/16 6:07 PM, David Holmes wrote:
> On 9/06/2016 10:54 AM, Ioi Lam wrote:
>> David,
>>
>> There are quite a few cases where resource-allocated buffers are
>> allocated. E.g.
>>
>>   const char* Symbol::as_klass_external_name() const;
>>
>> I don't think there's a rule against such usage. I think the rule should
>> be -- if a function is returning a resource-allocated buffer, don't do
>> the allocation within a ResourceMark declared by this function. This is
>> exactly what's done by Calvin's fix. It looks good to me.
>
> The problem is that rule has to propagate up the call chain and you 
> don't even know you are getting a resource-allocated buffer returned 
> in the first place! As it states in resourceArea.hpp:
>
> // The resource area holds temporary data structures in the VM.
> // The actual allocation areas are thread local. Typical usage:
> //
> //   ...
> //   {
> //     ResourceMark rm;
> //     int foo[] = NEW_RESOURCE_ARRAY(int, 64);
> //     ...
> //   }
> //   ...
>
> this is primarily for scoped data structures that you don't want to 
> try and fit in the stack. This is what the ResourceMarks enforce. If 
> we also use the resource-area as a per-thread heap. The two different 
> forms of usage are not compatible.
>
> In this case I'd like to see where the buffer eventually gets freed - 
> or whether it relies on the top-level ResourceMark to do that?
>

In this case, the buffer (which contains the classfile of the named 
class), is maintained by this ResourceMark:

    instanceKlassHandle ClassLoader::load_class(Symbol* name, bool
    search_append_only, TRAPS) {
       assert(name != NULL, "invariant");
       assert(THREAD->is_Java_thread(), "must be a JavaThread");

       ResourceMark rm(THREAD);


The code used to be simpler but has gradually grown in both depth and 
girth :-(

Thanks
- Ioi

> David
> -----
>
>> Thanks
>> - Ioi
>>
>> On 6/8/16 5:19 PM, David Holmes wrote:
>>> Hi Calvin,
>>>
>>> On 9/06/2016 4:01 AM, Calvin Cheung wrote:
>>>>
>>>> Please review this small fix for correcting the use of ResourceMark in
>>>> classLoader.cpp.
>>>>
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8159019
>>>> webrev: http://cr.openjdk.java.net/~ccheung/8159019/webrev.00/
>>>
>>> I think this really fixes the wrong problem. I don't think
>>> ClassPathZipEntry::open_entry should be returning a resource-allocated
>>> buffer in the first place. The lifecycle management is not suitable.
>>>
>>> David
>>> -----
>>>
>>>> Tested manually with the affected testcase on linux_x64.
>>>> Running tests on all platforms.
>>>>
>>>> thanks,
>>>> Calvin
>>