On Fri, 8 May 2026 19:28:12 GMT, Serguei Spitsyn <[email protected]> wrote:
>> I see three tests in that code: >> >> - `_jvf->method()->is_object_constructor()`: we're looking at an `<init>` >> method (I _think_ that's all this means?) >> - `_index == 0`: trying to limit the behavior to `this`, but local 0 might >> not be `this`, and a different local might be `this` >> - `obj != nullptr && obj->is_inline()`: the value of the local is a value >> object (again, guessing at what this code does) >> >> If I'm reading right, there's not test for a _value class_, but there is a >> test that the object is a _value object_. So that's the description I >> offered above: >> >>> When preview features are enabled, if the requested local is the "this" >>> variable >>> of a constructor, and it references a value object, the value_ptr is set to >>> a snapshot of the >>> value object. >> >> I'm concerned about "the this variable of a constructor", though, because >> that's not really what the code is doing. To more accurately specify it, it >> should be "if the 0th local variable of a constructor is requested". >> >> At that point we've got a spec and implementation in alignment. Great! But >> the the goal of the behavior has not been achieved, which is to defend >> against observable mutation of a larval value object (i.e., an >> `uninitializedThis` local variable that is a value object). I don't think >> you have any reliable way to tell which local variables might be `this`, and >> so to be safe we've got to cast a wider net and clone _all_ value objects >> accessed through a value class constructor. > >> so to be safe we've got to cast a wider net and clone all value objects >> accessed through a value class constructor. > > I like this suggestion. > > Then we need to reformulate the spec clarification to something like this: >> When preview features are enabled, the frame's method is a value class > constructor and the local is a value object, the value_ptr is set to a > snapshot of the > value object. > > The implementation can be corrected by replacing the check `_index == 0` with > the check `method->method_holder()->is_inline_class()` > My read on this is that if you are in a constructor, accessing local 0 > (this), and the current frame's method is for a value class, then the > snapshot will be done. Sorry, I misread the code. The "value" check is on the object, not the class. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2412#discussion_r3210963805
