Repost: RFR (S) JDK-6311046: -Xcheck:jni should support checking of GetPrimitiveArrayCritical

[Daniel D. Daugherty]

On 5/15/14 5:58 AM, David Simms wrote:
> Gidday all:
>
> Bug/Enhancement: https://bugs.openjdk.java.net/browse/JDK-6311046
>
> Web review: http://cr.openjdk.java.net/~dsimms/6311046/rev4/

src/share/vm/prims/jni.cpp
     No comments.

     Meta comment: Should there be a rhyme or reason to the order
     in which we add test calls in execute_internal_vm_tests().

src/share/vm/prims/jniCheck.cpp
     General: You have a number of new lines longer than 80 chars.
         Existing style in this file tries to stay below 80 (modulo
         the static string inits).

     line 343: tty->print_cr("%s: elements vector NULL" PTR_FORMAT, 
fn_name, obj);
         Should this a warning() call?
         Update: Looks like the style in this file is print_cr(). OK.

     line 1604: 
UNCHECKED()->Release##Result##ArrayElements(env,array,orig_result,mode); \
         Perhaps add spaces after the commmas like you've done elsewhere?


src/share/vm/runtime/os.cpp
     old line 743: if (memblock == NULL) {
     old line 744:   return malloc(size, memflags, (caller == 0 ? 
CALLER_PC : caller));
     new line 643: if (memblock == NULL) {
     new line 644:   return os::malloc(size, memflags, (caller == 0 ? 
CALLER_PC : caller));
         Looks like a case where NMT tracking was missing an
         allocation in non-ASSERT bits if os::realloc() was called
         with a NULL memblock parameter.

         Nice catch!

     old line 754: void* ptr = malloc(size, memflags, caller == 0 ? 
CALLER_PC : caller);
     new line 656: void* ptr = os::malloc(size, memflags, caller == 0 ? 
CALLER_PC : caller);
     old line 764: free(memblock);
     new line 669: os::free(memblock);
         And again. You should run these by Zhengyu so he can
         sanity check that this wasn't intentional and make sure
         that NMT2 has the same fix.

src/share/vm/memory/guardedMemory.hpp
     line 47:  * |+user_size         | 0xABABABABABABABAB   | Tail 
guard     |
         It might be better to use a different pattern for the
         tail guard. That way you'll be able to see the beginning
         and ending guards more easily in a memory dump.

     lines 62-64: code indents are off by one

     line 192 void* wrap_with_guards(..., const void* tag = 0) {
         "0" should be NULL; like the ctr

     line 193: assert(base_ptr, "Attempt to wrap NULL with memory guard");
     line 210: if (_base_addr) {
     line 236: assert(_base_addr, "Not wrapping any memory");
     line 246: assert(_base_addr, "Not wrapping any memory");
         Should be "base_ptr != NULL"; no implied booleans

     line 241: * Return the user data.
     line 243: * @return the user data.
         Returns a pointer to the user data, but not the user
         data itself.

     line 281: public:
         Needs blank line above and a single space before it.

src/share/vm/memory/guardedMemory.cpp
     line 25: #include "memory/guardedMemory.hpp"
         Why is this "#include" out of order and separated from
         the others?

     line 79: st->print_cr("  User data appears to be in use");
         Adding a "break" after this line will keep some of
         the tools from complaining about a "fall through".

     line 88: assert(p, "NULL pointer given to check");
         Should be "p != NULL"; no implied booleans.


Very nicely done. I like the encapsulation of this
sanity checking/helper code into guardedMemory.?pp.

Dan


>
> Cleaned up the "hand rolled" memory bounds checking in 
> os::malloc/realloc/free and type checking in checked JNI (GetString*), 
> and unified into a single helper class "GuardedMemory". Added some 
> extra checks to checked JNI (release mode).
>
> There is now some extra debugging support for free/release operations, 
> GuardedMemory::release_for_freeing()" will now mark user bytes with 
> "freeBlockPad", which did yield a result when testing.
>
> Testing Completed:
>
> Ran on all platforms:
>
>  * JPRT
>  * jteg jdk_core & jdk_svc
>  * "RT nightly".
>
>
> Cheers
> /David Simms
>
>