[lworld] RFR: 8253416: [lworld] C1: incorrect nestmate access to flattened field if nest-host is not loaded

[Frederic Parain]

On Thu, 24 Sep 2020 14:08:11 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> C1 incorrectly assumes that a nestmate field access is not flat if the nest-host is not loaded.
> 
> We are creating the ciField instance for a field in the nest-host via:
> `ciField::ciField -> Reflection::verify_member_access -> InstanceKlass::has_nestmate_access_to ->
> InstanceKlass::nest_host` Resolving the nest host then requires class loading which is not possible from the C1
> compiler thread and as a result we end up with partial field information (is_flattened is always false). The code in
> LIRGenerator::flattened_field_load_prolog then assumes that if !field->is_flattened(), the field is indeed not
> flattened. However, we can only rely on that information if !needs_patching.  I've completely re-wrote and simplified
> the complex logic in LIRGenerator::flattened_field_load_prolog and added asserts/checks to make sure we are not
> attempting to patch flattened inline type field accesses (that functionality could be added though but it's not easy).
> This patch also includes the following unrelated changes:
> - The "new" bytecode needs to throw an exception on inline klasses (the klass might be unloaded). The fix makes sure we
>   always take the slow path to "new_instance_no_inline" in that case.
> - The "defaultvalue" bytecode needs to throw an exception on non-inline klasses. We simply deoptimize in that case.
> - Load/stores from/to fields of an empty inline type should be removed.
> - Some refactoring and new asserts.

Tobias,

I'm still reviewing the code, but here are already two comments:

1 - the empty inline type optimization can be applied to load_indexed() and store_indexed() too.
2 - regarding the assert added at the beginning of copy_inline_content() (needs_patching must be false), it seems to me
that the method can still be called with a needs_patching argument being true from the GraphBuilder::withfield()
method. In the withfield() method, the needs_patching value is computed with this code:
        const bool needs_patching = !holder->is_loaded() ||
                              !field_modify->will_link(method(), Bytecodes::_withfield) ||
                              PatchALot;
And the value is passed as is to the copy_inline_content() method without consideration if the field being processed is
flattened or not.

Regards,

Fred

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

PR: https://git.openjdk.java.net/valhalla/pull/198