RFR: JDK-8080511 - Refresh of jimage support

[Jean-Francois Denise]

Hi Lois,

On 18 Jun 2015, at 19:55, Lois Foltan <lois.foltan at oracle.com> wrote:

> 
> On 6/18/2015 10:59 AM, Karen Kinnear wrote:
>> Code review for the hotspot component:
>> 
>> Thank you for the cleanups to jvm.cpp to make them JVM_ENTRY and fix thread_state.
>> 
>> If you pass the testing and Lois ok's the hotspot code, then I am ok with checking in the
>> hotspot code as is.
> 
> I have reviewed and am okay with checking in the hotspot code as is once testing is complete.  One comment inlined below.
> Lois
> 
>> 
>> Comments for next round of changes:
>> 
>> 1. mutexLocker.cpp - new lock should say "false" not "true" for whether the VMThread should block
>> 
>> 2. os.hpp
>> restartable_read_at is added but I don't see it used
>> 
>> 3. imageFile.cpp
>> We've been asked to always used the counted form of strcpy - i.e. strncpy  to ensure we never
>> have buffer overruns. e.g. 311, 437, ...
>> 
>> 4. jvm.cpp - seems odd that the interface would need to know if the resource was compressed
>> or uncompressed. In the next round of API design, seems like the java code would pass in
>> the uncompressedAddress and let the internals handle buffering the compressed data and
>> doing the uncompressing transparently.
>> 
>> 5. imageDecompressor.hpp
>> get_decompressor
>> I think you've mixed your exception handling approaches. You have a CHECK_NULL for new_symbol
>> call, which means it will throw an exception if it returns NULL. Then you check for HAS_PENDING_EXCEPTION
>> which is the logic you would use if did not have the CHECK, but then you throw the exception away.
>> If you want to throw an exception from get_decompressor you should pass in the last argument as TRAPS macro
>> which will give you the THREAD.
>> 
>> see macros in exceptions.hpp
>> 
>> So the way it is right now, you will throw an exception.
>> 
>> You have a choice - you can throw an exception
>>   pass in TRAPS to get_decompressor, keep the CHECK_NULL and remove the if HAS_PENDING_EXCEPTION.
>>   - this is the recommended approach
>> 
>> Alternative - change CHECK_NULL to pass in THREAD. Keep the rest. Given you are planning to take this
>> out of the vm - this would make more sense.
> 
> Whatever mechanism you choose to use in get_decompressor, please also use for image_decompressor_init( )/createSymbol() within imageDecompressor.cpp as well.

OK, well noted.

Thanks.
JF
> 
>> 
>> thanks,
>> Karen
>> 
>> 
>> 
>> On Jun 17, 2015, at 8:08 PM, Jim Laskey (Oracle) wrote:
>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8080511
>>> 
>>> This is an long overdue refresh of the jimage support in the JDK9-dev repo.  This includes native support for reading jimage files, improved jrt-fs (java runtime file system) support for retrieving modules and packages from the runtime, and improved performance for langtools in the presence of jrt-fs.
>>> 
>>> http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-top <http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-top>
>>> http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-jdk <http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-jdk>
>>> http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-hotspot <http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-hotspot>
>>> http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-langtools <http://cr.openjdk.java.net/~jlaskey/hs-rt-jimage/webrev-langtools>
>>> 
>>> 
>>> Details:
>>> 
>>> - jrt-fs provides access, via the nio FileSystem API, to the classes in a .jimage file, organized by module or by package.
>>> - Shared code for jimage support converted to native.  Currently residing in hotspot, but will migrate to it’s own jdk library https://bugs.openjdk.java.net/browse/JDK-8087181 <https://bugs.openjdk.java.net/browse/JDK-8087181>
>>> - A new archive abstraction for class/resource sources.
>>> - java based implementation layer for jimage reading to allow backport to JDK8 (jrt-fs.jar - IDE support.)
>>> - JNI support for jimage into hotspot.
>>> - White box tests written to exercise native jimage support.
>>> 
>