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 I have similar comments to Stefans. This is a nice change to differentiate the types and make it explicit that the runtime always uses RefArray. Also eliminates having to use CATCH/CHECK in callers for these runtime arrays. +1 vote for the overload of new_refArray. src/hotspot/share/cds/cdsProtectionDomain.cpp line 332: > 330: if (_shared_jar_manifests.resolve() == nullptr) { > 331: oop sjm = oopFactory::new_refArray( > 332: vmClasses::Jar_Manifest_klass(), size, > ArrayKlass::ArrayProperties::DEFAULT, CHECK); Since there have been several of these added, I would like to see a new oopFactory::new_default_refArray(), or even an overload of new_refArray without the DEFAULT properties. Internally in the JVM, they should all have default properties, so only called from some Java-facing api would they not be default. src/hotspot/share/cds/lambdaFormInvokers.cpp line 174: > 172: } > 173: > 174: refArrayHandle h_array(THREAD, > refArrayOopDesc::cast(result.get_oop())); Good to remove the direct C-style casts, even if this isn't all of them. We can incrementally fix more, then all of them later in mainline. src/hotspot/share/ci/ciArray.cpp line 66: > 64: { > 65: if (ary->is_refArray()) { > 66: assert(ary->is_objArray(), ""); Is this not given by the type system so unnecessary to assert? src/hotspot/share/ci/ciArray.cpp line 76: > 74: JavaThread* THREAD = CompilerThread::current(); > 75: oop elem = flatary->obj_at(index, THREAD); > 76: if (HAS_PENDING_EXCEPTION) { This is odd. The compiler cannot create exceptions because it can't call Java code (nor can it allocate?). Should this be CATCH instead? src/hotspot/share/memory/oopFactory.cpp line 123: > 121: refArrayOop oopFactory::new_refArray(Klass* klass, int length, > ArrayKlass::ArrayProperties properties, TRAPS) { > 122: ArrayKlass* array_type = klass->array_klass(CHECK_NULL); > 123: ObjArrayKlass* oak = > ObjArrayKlass::cast(array_type)->klass_with_properties(properties, true, > CHECK_NULL); I'm having trouble understanding this "force_refarray" parameter. ------------- Changes requested by coleenp (Committer). PR Review: https://git.openjdk.org/valhalla/pull/2033#pullrequestreview-3764789550 PR Comment: https://git.openjdk.org/valhalla/pull/2033#issuecomment-3873970169 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2775692042 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2775697051 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2775700880 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2775707974 PR Review Comment: https://git.openjdk.org/valhalla/pull/2033#discussion_r2775752262
