Emit `readonly.` for constrained array access #70949

jjonescz · 2023-11-23T14:57:16Z

jaredpar · 2023-11-27T22:03:18Z

src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenCallTests.cs

-  IL_0043:  constrained. ""T""
-  IL_0049:  callvirt   ""void IMoveable.this[int].set""
-  IL_004e:  ret
+  IL_0006:  readonly.


I'm a bit warry of generating unverifiable IL. It's necessary sometimes, particularly with newer constructs that were created after verification was frozen. That doesn't seem to be the case for this.

I'm not as familiar with the .readonly prefix and had to look it up for this review. The set of allowed instructions does not include stloc / ldloc. Wondering if that is an ommision or not. The subsequent operations we perform after ldloc seem to fall into the allowed set.

I'd prefer to avoid this if possible. This is the opcodes that we generate today for the method:

.maxstack 8 IL_0000: nop IL_0001: ldarg.0 IL_0002: ldc.i4.0 IL_0003: ldelema !!T IL_0008: dup IL_0009: constrained. !!T IL_000f: callvirt instance int32 I1::get_P() IL_0014: ldc.i4.2 IL_0015: add IL_0016: constrained. !!T IL_001c: callvirt instance void I1::set_P(int32) IL_0021: nop IL_0022: ret

Where is the ArrayTypeMismatchException being generated today? Is it on the ldelema instruction because the the type of the array is Base but the elements are Derived?

@davidwrighton in case he has any thoughts on how to best approach this.

Where is the ArrayTypeMismatchException being generated today? Is it on the ldelema instruction because the the type of the array is Base but the elements are Derived?

Yes.

The set of allowed instructions does not include stloc / ldloc

We are not emitting readonly. for stloc/ldloc in this PR, I believe, only for ldelema.

I'm a bit warry of generating unverifiable IL. It's necessary sometimes, particularly with newer constructs that were created after verification was frozen. That doesn't seem to be the case for this.

Yeah, ok, that sounds good. We can instead spill the array access in various ways described in #70267. I will look into that.

jaredpar · 2023-11-27T22:06:59Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs

            {
-                resultExpr = base.VisitArrayAccess(node)!;
+                resultExpr = arr.Update(arr.Expression, arr.Indices, inCompoundAssignmentReceiver: true, arr.Type);


Do we only set inCompoundAssignmentReceiver when the element type is a type parameter? If so then I think we should rename that property such that it captures. Based on the current name I was confused looking at the changes to EmitAddress.cs as to why more baselines didn't change to have the .readonly prefix on them.

jaredpar · 2023-11-27T22:14:09Z

...Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CompoundAssignmentOperator.cs

+            var previousInCompoundAssignmentReceiver = _inCompoundAssignmentReceiver;
+            _inCompoundAssignmentReceiver |= isRegularCompoundAssignment;
+            var loweredReceiver = VisitExpression(receiver);
+            _inCompoundAssignmentReceiver = previousInCompoundAssignmentReceiver;
+            return loweredReceiver;


It seems like _inCompoundAssignmentReceiver will be set for the duration of the VisitExpression call. Doesn't that mean that all array accesses within the receiver will end up with the .readonly prefix? Essentially I'm thinking about the following:

array1[(array2[0]=someValue).Index].Prop += 42;

Will the inner array2[0] get the .readonly prefix? If so that seems incorrect and would lead to array mismatch bugs.

AlekseyTs · 2023-11-30T02:37:16Z

It looks like we already deal with a similar situation in context of a compound assignment to an array element without emitting an unverifiable code. See usages of SpillArrayElementAccess helper. I there really a good reason to do something different for other scenarios?

jjonescz · 2023-12-01T13:46:49Z

It looks like we already deal with a similar situation in context of a compound assignment to an array element without emitting an unverifiable code. See usages of SpillArrayElementAccess helper. I there really a good reason to do something different for other scenarios?

I'm investigating using SpillArrayElementAccess. But for example nested array access is much simpler than general indexer (since it's limited to arrays, it can emit ldelem_ref since arrays are reference types, whereas the nested indexer can be a struct).

AlekseyTs · 2023-12-01T16:42:12Z

But for example nested array access is much simpler than general indexer (since it's limited to arrays, it can emit ldelem_ref since arrays are reference types, whereas the nested indexer can be a struct).

Sorry, I am not following. It looks to me that SpillArrayElementAccess usage is limited to arrays. I am also not sure what "general indexer" term refers to in this context.
In addition consider the following:

If I remember correctly, VB compiler successfully deals with the same problematic scenarios without violating IL verification rules.
I think that async spilling rewriter in C# also deals with similar situations without violating IL verification rules.

Similar situations are situations when an array element access is used as a receiver of a method call and it is getting spilled (i.e. its side- effects are evaluated at one point of time and the actual access is performed at a later time, quite possibly more than once).

AlekseyTs · 2023-12-01T16:59:26Z

I would like to bring your attention to the following quote from the issue:

Other scenarios where compiler captures references to array elements, which could be reference types, could be affected as well.

I am pretty sure that affected scenarios are not limited to compound assignment scenarios, while they are the most obvious/simple examples. I would scope affected scenarios to scenarios for which LocalRewriter captures receivers of method calls in a ByRef local. Those places should be easily identifiable based on the helper used for that.

AlekseyTs · 2023-12-01T17:06:36Z

...Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CompoundAssignmentOperator.cs

@@ -664,7 +664,9 @@ private BoundExpression TransformCompoundAssignmentLHS(BoundExpression originalL
                            //   I t_index = index; (possibly more indices)
                            //   T value = t_array[t_index];
                            //   t_array[t_index] = value op R;
-                            var loweredArray = VisitExpression(arrayAccess.Expression);
+
+                            var loweredArray = VisitCompoundAssignmentReceiver(arrayAccess.Expression, isRegularCompoundAssignment);


var loweredArray = VisitCompoundAssignmentReceiver(arrayAccess.Expression, isRegularCompoundAssignment);

I think that this code path doesn't need any special handling. The SpillArrayElementAccess call below should already handle things properly.

AlekseyTs · 2023-12-01T17:15:21Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs

            {
-                resultExpr = base.VisitArrayAccess(node)!;
+                resultExpr = arr.Update(arr.Expression, arr.Indices, inCompoundAssignmentReceiver: true, arr.Type);


inCompoundAssignmentReceiver: true

I think that, in a general case, this assumption is incorrect. The expression that we are rewriting while _inCompoundAssignmentReceiver is set to true could be arbitrarily complex, with any number of array access expressions used in it in any number of positions that are not receiver positions. More over, the array access can be a target of an explicit ref taking operation, like right hand side of a ref assignment, or a ref argument, etc.

To clarify. Strictly speaking, yes, the array access is in a compound assignment receiver. However, the value of inCompoundAssignmentReceiver is used by emit layer as an indicator that using readonly. prefix is appropriate. Which is incorrect in a general case, even if we put the IL verification issue aside.

jjonescz · 2023-12-04T11:37:27Z

I think that async spilling rewriter in C# also deals with similar situations without violating IL verification rules.

Similar situations are situations when an array element access is used as a receiver of a method call and it is getting spilled (i.e. its side- effects are evaluated at one point of time and the actual access is performed at a later time, quite possibly more than once).

Oh, thanks, I didn't think of looking into this scenario. Seems that it's not behaving as I would expect though.

Your test GenericTypeParameterAsReceiver_Assignment_Compound_Indexer_Struct_Index_Async_01_ThroughArray has a different output than GenericTypeParameterAsReceiver_Assignment_Compound_Indexer_Class_Index_Async_01_ThroughArray. Shouldn't they have the same output? I thought the whole point in expression like array[index1][index2] += 1 is to evaluate reference to array[index1] prior to index2 so side effects of index2 would not affect it. The class scenario does that, the struct scenario doesn't.

If I remember correctly, VB compiler successfully deals with the same problematic scenarios without violating IL verification rules.

Yes, that's true. VB checks at runtime if T is a class or a struct (default(T) == null) via EmitComplexConditionalAccessReceiver. Seems that this exists in C# as well, I will try to use it.

Sorry, I am not following. It looks to me that SpillArrayElementAccess usage is limited to arrays. I am also not sure what "general indexer" term refers to in this context.

Today, SpillArrayElementAccess is used in nested array access scenarios like array[index1][index2] to spill the array[index1] part. But using it in indexer access with array access receiver results in wrong order of operations (index2 is evaluated prior to array[index1]).

AlekseyTs · 2023-12-04T15:09:51Z

@jjonescz I think it is time we sync up offline.

AlekseyTs · 2023-12-04T15:13:21Z

But using it in indexer access with array access receiver results in wrong order of operations (index2 is evaluated prior to array[index1]).

I have absolutely no idea what exactly you are doing, and, therefore, cannot comment on results that you are getting and why you are getting them.

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 23, 2023

jjonescz force-pushed the 70267-ArrayTypeMismatch-ro branch from 7f0f874 to 8b514fe Compare November 24, 2023 10:38

jjonescz changed the title ~~Emit constrained array access without runtime type check~~ Emit readonly. for constrained array access Nov 24, 2023

Emit readonly. for constrained array access

cd954d0

jjonescz force-pushed the 70267-ArrayTypeMismatch-ro branch from 8b514fe to cd954d0 Compare November 24, 2023 10:54

jjonescz marked this pull request as ready for review November 24, 2023 12:20

jjonescz requested a review from a team as a code owner November 24, 2023 12:20

jaredpar reviewed Nov 27, 2023

View reviewed changes

jjonescz marked this pull request as draft November 28, 2023 09:19

AlekseyTs reviewed Dec 1, 2023

View reviewed changes

jjonescz closed this Dec 7, 2023

jjonescz deleted the 70267-ArrayTypeMismatch-ro branch December 7, 2023 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit `readonly.` for constrained array access #70949

Emit `readonly.` for constrained array access #70949

jjonescz commented Nov 23, 2023 •

edited

Loading

jaredpar Nov 27, 2023

jjonescz Nov 28, 2023

jaredpar Nov 27, 2023

jaredpar Nov 27, 2023

AlekseyTs commented Nov 30, 2023

jjonescz commented Dec 1, 2023

AlekseyTs commented Dec 1, 2023 •

edited

Loading

AlekseyTs commented Dec 1, 2023

AlekseyTs Dec 1, 2023

AlekseyTs Dec 1, 2023

AlekseyTs Dec 1, 2023

jjonescz commented Dec 4, 2023 •

edited

Loading

AlekseyTs commented Dec 4, 2023

AlekseyTs commented Dec 4, 2023

Emit readonly. for constrained array access #70949

Emit readonly. for constrained array access #70949

Conversation

jjonescz commented Nov 23, 2023 • edited Loading

jaredpar Nov 27, 2023

Choose a reason for hiding this comment

jjonescz Nov 28, 2023

Choose a reason for hiding this comment

jaredpar Nov 27, 2023

Choose a reason for hiding this comment

jaredpar Nov 27, 2023

Choose a reason for hiding this comment

AlekseyTs commented Nov 30, 2023

jjonescz commented Dec 1, 2023

AlekseyTs commented Dec 1, 2023 • edited Loading

AlekseyTs commented Dec 1, 2023

AlekseyTs Dec 1, 2023

Choose a reason for hiding this comment

AlekseyTs Dec 1, 2023

Choose a reason for hiding this comment

AlekseyTs Dec 1, 2023

Choose a reason for hiding this comment

jjonescz commented Dec 4, 2023 • edited Loading

AlekseyTs commented Dec 4, 2023

AlekseyTs commented Dec 4, 2023

Emit `readonly.` for constrained array access #70949

Emit `readonly.` for constrained array access #70949

jjonescz commented Nov 23, 2023 •

edited

Loading

AlekseyTs commented Dec 1, 2023 •

edited

Loading

jjonescz commented Dec 4, 2023 •

edited

Loading