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

[Tobias Hartmann]

On Thu, 24 Sep 2020 20:45:54 GMT, Frederic Parain <fparain 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

Hi Fred,

thanks for the quick review!

> 1 - the empty inline type optimization can be applied to load_indexed() and store_indexed() too.

Yes, there are even more optimization opportunities and I'll address these with
[JDK-8248220](https://bugs.openjdk.java.net/browse/JDK-8248220). I've only added the empty field load/store removal
here for correctness because otherwise we miss adding a null check (see re-enabled `TestLWorld::test122` and
`test123`). The "Empty inline type access should be removed" assert in `GraphBuilder::copy_inline_content` would now
catch this.

> 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.

If `needs_patching` is true, we emit a `WithField` node that triggers deoptimization, see
[here](https://github.com/openjdk/valhalla/pull/198/files#diff-46a0868648fa7bf1ee142760243a6d86R2049). That code will
be re-visited by [JDK-8228634](https://bugs.openjdk.java.net/browse/JDK-8228634).

Thanks,
Tobias

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

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