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

[David Holmes]

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?

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
>