On Thu, 10 Sep 2026 11:02:17 GMT, Stefan Karlsson <[email protected]> wrote:
>>> Unfortunately, this doesn't work. I actually tried this first, passing the >>> offset only instead of the vpc and relying on the field_holder(), but that >>> is wrong. The code could be visiting a field inside an abstract value class >>> and when that happens field_holder() doesn't match vpc.klass(). It is easy >>> to trigger the assert by writing a test that creates an abstract value >>> class with a field, then a concrete value class that inherits from it, put >>> it in a holder object, and then route the holder into the print fields code. >> >> Thanks for the clarification. I restored `ValuePayloadContext::klass()` and >> used it inside `fieldDescriptor::field_offset_in_obj()`. >> >> I also added an assert to clarify the relationship between `vpc->klass()` >> and `this->field_holder()`. >> >> We could actually get rid of `klass()`, as `vpc->klass()->payload_offset()` >> must be the same as >> `ValueKlass::cast(this->field_holder())->payload_offset()`. However, I think >> that requires too much mental gymnastics and it's not worthwhile in this >> case. > >> > Unfortunately, this doesn't work. I actually tried this first, passing the >> > offset only instead of the vpc and relying on the field_holder(), but that >> > is wrong. The code could be visiting a field inside an abstract value >> > class and when that happens field_holder() doesn't match vpc.klass(). It >> > is easy to trigger the assert by writing a test that creates an abstract >> > value class with a field, then a concrete value class that inherits from >> > it, put it in a holder object, and then route the holder into the print >> > fields code. >> >> Thanks for the clarification. I restored `ValuePayloadContext::klass()` and >> used it inside `fieldDescriptor::field_offset_in_obj()`. >> >> I also added an assert to clarify the relationship between `vpc->klass()` >> and `this->field_holder()`. >> >> We could actually get rid of `klass()`, as `vpc->klass()->payload_offset()` >> must be the same as >> `ValueKlass::cast(this->field_holder())->payload_offset()`. However, I think >> that requires too much mental gymnastics and it's not worthwhile in this >> case. > > Note that `ValueKlass::cast(this->field_holder())` will fail because abstract > value classes are represented with an `InstanceKlass` and not a `ValueKlass`. Thanks @stefank @fparain @matias9927 for the review I merged to resolve conflicts. Mach5 tests are clean. ------------- PR Comment: https://git.openjdk.org/jdk/pull/32565#issuecomment-5720217117
