[interp] Add wide data item index static load and stores #59220

lambdageek · 2021-09-16T19:08:37Z

Add ldsdfld.w and stsfld.w opcodes to the interpreter. These are needed for some machine-generated methods in Android projects that use AndroidX, MAUI, SkiaSharp or other toolkits. The app's Resource::UpdateIdValues can grow to exceed 64k interpreter data items, which means that 16-bit data item indices will roll over.

A typical snippet from the method body:

    IL_8413:  ldc.i4     0x7f0f02fc
    IL_8418:  stsfld     int32 [Microsoft.Maui.Controls.Compatibility]Microsoft.Maui.Controls.Compatibility.Resource/Style::Widget_MaterialComponents_Toolbar_Surface
    IL_841d:  ldc.i4     0x7f0f02fd
    IL_8422:  stsfld     int32 [Microsoft.Maui.Controls.Compatibility]Microsoft.Maui.Controls.Compatibility.Resource/Style::Widget_MaterialComponents_Tooltip
    IL_8427:  ldc.i4     0x7f0f02fe
    IL_842c:  stsfld     int32 [Microsoft.Maui.Controls.Compatibility]Microsoft.Maui.Controls.Compatibility.Resource/Style::Widget_Support_CoordinatorLayout
    IL_8431:  ldsfld     int32[] TestApp.Resource/Styleable::ActionBar
    IL_8436:  stsfld     int32[] [Microsoft.Maui.Controls.Compatibility]Microsoft.Maui.Controls.Compatibility.Resource/Styleable::ActionBar
    IL_843b:  ldsfld     int32[] TestApp.Resource/Styleable::ActionBarLayout
    IL_8440:  stsfld     int32[] [Microsoft.Maui.Controls.Compatibility]Microsoft.Maui.Controls.Compatibility.Resource/Styleable::ActionBarLayout
    IL_8445:  ldc.i4.0
    IL_8446:  stsfld     int32 [Microsoft.Maui.Controls.Compatibility]Microsoft.Maui.Controls.Compatibility.Resource/Styleable::ActionBarLayout_android_layout_gravity

The "wide" opcodes use 32-bit data item indices.

Resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1398069

For methods that require more than 64k data items, add a variant stsfld instruction that stores data item indices as 32-bit integers instead of 16-bit. To solve this problem in general, we need wide variants of more opcodes, but for specific (machine generated) methods, a wide stsfld instruction is sufficient. Resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1398069

ghost · 2021-09-16T19:08:44Z

Tagging subscribers to this area: @BrzVlad
See info in area-owners.md if you want to be subscribed.

Issue Details

Add ldsdfld.vt.w and stsfld.vt.w opcodes to the interpreter. These are needed for some machine-generated methods in Android projects that use AndroidX, MAUI, SkiaSharp or other toolkits. The app's Resource::UpdateIdValues can grow to exceed 64k interpreter data items, which means that 16-bit data item indices will roll over.

A typical snippet from the method body:

    IL_8413:  ldc.i4     0x7f0f02fc
    IL_8418:  stsfld     int32 [Microsoft.Maui.Controls.Compatibility]Microsoft.Maui.Controls.Compatibility.Resource/Style::Widget_MaterialComponents_Toolbar_Surface
    IL_841d:  ldc.i4     0x7f0f02fd
    IL_8422:  stsfld     int32 [Microsoft.Maui.Controls.Compatibility]Microsoft.Maui.Controls.Compatibility.Resource/Style::Widget_MaterialComponents_Tooltip
    IL_8427:  ldc.i4     0x7f0f02fe
    IL_842c:  stsfld     int32 [Microsoft.Maui.Controls.Compatibility]Microsoft.Maui.Controls.Compatibility.Resource/Style::Widget_Support_CoordinatorLayout
    IL_8431:  ldsfld     int32[] TestApp.Resource/Styleable::ActionBar
    IL_8436:  stsfld     int32[] [Microsoft.Maui.Controls.Compatibility]Microsoft.Maui.Controls.Compatibility.Resource/Styleable::ActionBar
    IL_843b:  ldsfld     int32[] TestApp.Resource/Styleable::ActionBarLayout
    IL_8440:  stsfld     int32[] [Microsoft.Maui.Controls.Compatibility]Microsoft.Maui.Controls.Compatibility.Resource/Styleable::ActionBarLayout
    IL_8445:  ldc.i4.0
    IL_8446:  stsfld     int32 [Microsoft.Maui.Controls.Compatibility]Microsoft.Maui.Controls.Compatibility.Resource/Styleable::ActionBarLayout_android_layout_gravity

The "wide" opcodes use 32-bit data item indices.

Resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1398069

Author:	lambdageek
Assignees:	-
Labels:	`area-Codegen-Interpreter-mono`
Milestone:	-

lambdageek · 2021-09-16T19:17:11Z

For .NET 7.0 we should investigate how to fix this in general - I made an issue #59221.

This version that just deals with UpdateIdValues methods should be backported to .NET 6.0

src/mono/mono/mini/interp/interp.c

src/mono/mono/mini/interp/transform.c

Use stackval_from_data and stackval_to_data instead of memcpy. Don't duplicate code get_data_item_wide_index. Only use the wide opcodes if any of the data item indices of a given instruction need it, not just when the method total number of indices overflows.

lambdageek · 2021-09-17T02:05:41Z

Ran some local tests always using the wide opcodes and it seems like it's working. But I'd appreciate another look at the stackval_from_data/stackval_to_data in interp.c @BrzVlad

src/mono/mono/mini/interp/mintops.def

src/mono/mono/mini/interp/transform.c

BrzVlad

Looks good. Just those last nitpicks

lambdageek · 2021-09-17T13:03:59Z

/backport to release/6.0-rc2

github-actions · 2021-09-17T13:04:12Z

Started backporting to release/6.0-rc2: https://github.com/dotnet/runtime/actions/runs/1245567304

…tores (#59262) Backport of #59220 Add ldsdfld.w and stsfld.w opcodes to the interpreter. These are needed for some machine-generated methods in Android projects that use AndroidX, MAUI, SkiaSharp or other toolkits. The app's Resource::UpdateIdValues can grow to exceed 64k interpreter data items, which means that 16-bit data item indices will roll over. Resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1398069

lambdageek added 5 commits September 16, 2021 15:03

warn if there's a ton of data items

d1bfe67

Add ldsfld.vt.w opcode

d954010

[mono] Log debug message when running .cctors

13c2a2c

drop hacky logging

dfd169f

lambdageek requested review from BrzVlad, thaystg and vargaz as code owners September 16, 2021 19:08

ghost added the area-Codegen-Interpreter-mono label Sep 16, 2021

lambdageek mentioned this pull request Sep 16, 2021

[interp] Support methods that need more than 64k data items #59221

Open

fix whitespace

d262515

BrzVlad reviewed Sep 16, 2021

View reviewed changes

src/mono/mono/mini/interp/interp.c Outdated Show resolved Hide resolved

BrzVlad reviewed Sep 16, 2021

View reviewed changes

src/mono/mono/mini/interp/transform.c Outdated Show resolved Hide resolved

BrzVlad reviewed Sep 16, 2021

View reviewed changes

src/mono/mono/mini/interp/transform.c Outdated Show resolved Hide resolved

BrzVlad reviewed Sep 16, 2021

View reviewed changes

src/mono/mono/mini/interp/transform.c Outdated Show resolved Hide resolved

BrzVlad reviewed Sep 16, 2021

View reviewed changes

src/mono/mono/mini/interp/transform.c Outdated Show resolved Hide resolved

lambdageek force-pushed the fix-androidx branch 3 times, most recently from f06ce51 to 3ee598f Compare September 17, 2021 00:51

Address review feedback

2683a98

Use stackval_from_data and stackval_to_data instead of memcpy. Don't duplicate code get_data_item_wide_index. Only use the wide opcodes if any of the data item indices of a given instruction need it, not just when the method total number of indices overflows.

lambdageek force-pushed the fix-androidx branch from 3ee598f to 2683a98 Compare September 17, 2021 02:02

BrzVlad reviewed Sep 17, 2021

View reviewed changes

src/mono/mono/mini/interp/mintops.def Outdated Show resolved Hide resolved

BrzVlad reviewed Sep 17, 2021

View reviewed changes

src/mono/mono/mini/interp/transform.c Outdated Show resolved Hide resolved

BrzVlad reviewed Sep 17, 2021

View reviewed changes

src/mono/mono/mini/interp/transform.c Outdated Show resolved Hide resolved

BrzVlad approved these changes Sep 17, 2021

View reviewed changes

lambdageek added 2 commits September 17, 2021 08:44

Fix wide index boundary conditions

9d4033c

Rename opcodes to ldsfld.w and stsfld.w

847d56a

github-actions bot mentioned this pull request Sep 17, 2021

[release/6.0-rc2] [interp] Add wide data item index static load and stores #59262

Merged

lambdageek merged commit f5e1baa into dotnet:main Sep 17, 2021

ghost locked as resolved and limited conversation to collaborators Nov 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[interp] Add wide data item index static load and stores #59220

[interp] Add wide data item index static load and stores #59220

Uh oh!

lambdageek commented Sep 16, 2021 •

edited

Loading

Uh oh!

ghost commented Sep 16, 2021

Uh oh!

lambdageek commented Sep 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdageek commented Sep 17, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BrzVlad left a comment

Uh oh!

lambdageek commented Sep 17, 2021

Uh oh!

github-actions bot commented Sep 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[interp] Add wide data item index static load and stores #59220

[interp] Add wide data item index static load and stores #59220

Uh oh!

Conversation

lambdageek commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 16, 2021

Uh oh!

lambdageek commented Sep 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdageek commented Sep 17, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BrzVlad left a comment

Choose a reason for hiding this comment

Uh oh!

lambdageek commented Sep 17, 2021

Uh oh!

github-actions bot commented Sep 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lambdageek commented Sep 16, 2021 •

edited

Loading