RFR: 8295486: Inconsistent constant field values observed during compilation [v5]

[Vladimir Ivanov]

On Fri, 20 Jan 2023 08:11:49 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> As it is now, it doesn't help static/instance fields, but it can be improved. Something for a follow-up cleanup.
> Could you elaborate? Final static/instance fields are handled as well (ciField::constant_value_of -> ciInstance::field_value -> ciInstance::field_value_impl). Do you mean non-final fields?

Not necessarily. I spotted the following for static final fields:

ciConstant ciField::constant_value() {
  ...
  if (_constant_value.basic_type() == T_ILLEGAL) {
    // Static fields are placed in mirror objects.
    VM_ENTRY_MARK;
    ciInstance* mirror = CURRENT_ENV->get_instance(_holder->get_Klass()->java_mirror());
    _constant_value = mirror->field_value_impl(type()->basic_type(), offset());
  }

As it is now, you can't avoid the state transition when doing `field_value_impl()` call. 
You could reshape it into:

  if (_constant_value.basic_type() == T_ILLEGAL) {
    // Static fields are placed in mirror objects.
    ciInstance* mirror = _holder->java_mirror();
    _constant_value = mirror->field_value_impl(type()->basic_type(), offset());
  }
``` 
... and then put a call to check the cache first before trying to load a value from the field. There should be a `VM_ENTRY_MARK` in `field_value_impl()`, but the check can be placed before it. Then `ciInstance::field_value()` won't need `GUARDED_VM_ENTRY` when calling into `field_value_impl()`.

> I reproduced it. The problem is the new call to verify_type that I added to PhaseCCP::verify_analyze.

Nice catch!

-------------

PR: https://git.openjdk.org/jdk/pull/11861