On Mon, 9 Feb 2026 19:47:00 GMT, Frederic Parain <[email protected]> wrote:
>> First batch of changes to remove potentially dangerous calls to >> objArrayOopDesc::obj_at(). >> Changes are more extensive than intended. In most cases, code modifications >> consist in using a refArrayOop type instead of a objArrayOop type, because >> most of the arrays the JVM deals with for its own purpose are always >> reference arrays (because they are arrays of identity type elements). The >> patch also adds a new API allowing the VM to request the allocation of a >> reference array. >> Code dealing with user provided arrays must be ready to handle exceptions >> when accessing objArrays. >> >> This is a short term fix, fixing a few bugs, and trying to make the code >> more robust using the meta-data types. For the long term, a better solution >> is needed. Accesses to both arrays and fields are becoming more and more >> complex because of the introduction of flattening, multiple layouts, >> additional properties. Forcing enforcement at each access would be expensive >> and wasteful, as the JVM usually operates on well-known objects or arrays. >> But because of the increasing complexity, having a way to quickly check the >> validity of an access would help making the VM code more robust. > > Frederic Parain has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 12 additional > commits since the last revision: > > - Comments update > - Rename new_default_refArray to new_refArray with overload > - Fixes issues mentioned in reviews > - Remove force_refarray and add array klass creation from ArrayDescription > - Fix merge > - Merge remote-tracking branch 'upstream/lworld' into refarray_factory > - Revert foreign methods signature changes > - Foreign API and other fixes > - CI fixes > - More fixes in serviceability code > - ... and 2 more: > https://git.openjdk.org/valhalla/compare/dbed07f5...bd5f33d0 Given that "default" ref arrays is going to be the most common type of array we create in the runtime code I would suggest that you revert the name `oopFactory::new_default_refArray` to nicer `oopFactory::new_refArray` name, and then we can have a longer name for the when we need to add some extra properties (E.g. oopFactory::new_refArray_with_properties`). I've added comments about nits below. src/hotspot/share/memory/oopFactory.cpp line 135: > 133: } > 134: > 135: Suggestion: } src/hotspot/share/memory/oopFactory.hpp line 63: > 61: // Factory forcing the creation of a reference array > 62: static refArrayOop new_refArray(Klass* klass, int length, > ArrayKlass::ArrayProperties properties, TRAPS); > 63: static refArrayOop new_default_refArray(Klass* klass, int length, > TRAPS); Swap order to match the block above (alt. swap the order above). Suggestion: static refArrayOop new_default_refArray(Klass* klass, int length, TRAPS); static refArrayOop new_refArray(Klass* klass, int length, ArrayKlass::ArrayProperties properties, TRAPS); src/hotspot/share/memory/universe.hpp line 132: > 130: > 131: // the array of preallocated errors with backtraces > 132: static refArrayOop preallocated_out_of_memory_errors(); While your at it ... Suggestion: static refArrayOop preallocated_out_of_memory_errors(); src/hotspot/share/oops/flatArrayOop.hpp line 43: > 41: void* value_at_addr(int index, jint lh) const; > 42: > 43: inline static flatArrayOop cast(oop o); I wonder when we should have these *OopDesc casts and when we should use the oopCast.hpp functions? src/hotspot/share/oops/flatArrayOop.inline.hpp line 59: > 57: > 58: inline oop flatArrayOopDesc::obj_at(int index) const { > 59: fatal("Should not reach here"); Maybe: Suggestion: fatal("Should not be used with flat arrays"); src/hotspot/share/oops/klass.hpp line 61: > 59: class PackageEntry; > 60: class vtableEntry; > 61: class ArrayDescription; Would be nice if you inserted this sorted. src/hotspot/share/oops/klass.hpp line 625: > 623: #ifdef ASSERT > 624: void validate_array_description(ArrayDescription ad); > 625: #endif // ASSERT Suggestion: void validate_array_description(ArrayDescription ad) NOT_DEBUG_RETURN; We also tend to try to keep verification code at the end of the oops/ classes, so maybe it would be good too place it there as well. src/hotspot/share/oops/objArrayKlass.cpp line 181: > 179: // TODO FIXME: the layout selection should take the array size in > consideration > 180: // to avoid creation of arrays too big to be handled by the VM. See > JDK-8233189 > 181: if (!UseArrayFlattening || element->is_array_klass() || > element->is_identity_class()|| element->is_abstract()) { Suggestion: if (!UseArrayFlattening || element->is_array_klass() || element->is_identity_class() || element->is_abstract()) { src/hotspot/share/oops/objArrayKlass.cpp line 412: > 410: } > 411: > 412: ObjArrayKlass* ObjArrayKlass::klass_from_description(ArrayDescription > adesc, TRAPS) { `adesc` is some sort of a mix because an abbreviation and a shortening of a word, and it is unconventional in our code base. Maybe just use `ad` for it and change the other `ad` to a name with a more explicit name that indicates why you need to figure out a new `ArrayDescription`? src/hotspot/share/oops/objArrayKlass.cpp line 416: > 414: #ifdef ASSERT > 415: element_klass()->validate_array_description(adesc); > 416: #endif // ASSERT If you took the proposed `NOT_DEBUG_RETURN` change then this could be changed to: Suggestion: element_klass()->validate_array_description(adesc); src/hotspot/share/oops/objArrayKlass.cpp line 439: > 437: release_set_next_refined_klass(first); > 438: } > 439: ak = allocate_klass_from_description(adesc, THREAD); Pre-existing but still weird: `ak` is set here and then unconditionally overwritten further down. I'd suggest that you use another local variable to hold this klass. src/hotspot/share/opto/runtime.cpp line 369: > 367: // Null-free arrays need to be initialized > 368: #ifdef ASSERT > 369: ObjArrayKlass* oak = ObjArrayKlass::cast(result->klass()); `oak` shadows the earlier `oak`, which makes the code harder to read. src/hotspot/share/opto/runtime.cpp line 372: > 370: assert(oak->is_null_free_array_klass(), "Sanity check"); > 371: #endif > 372: // assert(!h_init_val.is_null(), "Cannot initialize null free > array with null"); This was already checked above Suggestion: src/hotspot/share/prims/jvm.cpp line 1452: > 1450: > 1451: // Allocate temp. result array > 1452: refArrayOop r = > oopFactory::new_default_refArray(vmClasses::Class_klass(), length/4, > CHECK_NULL); Suggestion: refArrayOop r = oopFactory::new_default_refArray(vmClasses::Class_klass(), length / 4, CHECK_NULL); src/hotspot/share/prims/jvm.cpp line 1453: > 1451: // Allocate temp. result array > 1452: refArrayOop r = > oopFactory::new_default_refArray(vmClasses::Class_klass(), length/4, > CHECK_NULL); > 1453: refArrayHandle result (THREAD, r); Suggestion: refArrayHandle result(THREAD, r); ------------- PR Review: https://git.openjdk.org/valhalla/pull/2033#pullrequestreview-3772241338 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781611025 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781615775 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781619958 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781638411 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781642310 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781649379 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781667160 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781668703 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781707595 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781680376 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781744425 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781762480 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781765171 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781771456 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2781772506
