Added real support for Unbox stubs to WebAssembly by hippiehunter · Pull Request #5052 · dotnet/corert

hippiehunter · 2017-12-01T21:49:07Z

This fixes #5005
@MichalStrehovsky I think this is in line with your first suggested path, but I'm a little hazy on the real meaning of things like the ClassCode when adding a new node. I'm also not totally sure that I really needed to use BoxedValueType at all, I think maybe I could have just used the original value type.

MichalStrehovsky · 2017-12-01T22:09:28Z

I'm also not totally sure that I really needed to use BoxedValueType at all, I think maybe I could have just used the original value type.

If you make a valuetype the parent of the thunk, the thunk would be an instance method on a valuetype. this for instance methods on valuetypes is a byref. this for classes (and boxed valuetypes) is an object reference. It would mess up the GC reporting within the method, among other things.

MichalStrehovsky · 2017-12-01T22:27:32Z

Do we have enough things working that you could write a test that boxes a valuetype and then e.g. calls Equals?

MichalStrehovsky · 2017-12-01T22:11:29Z

+            // Get a reference type that has the same layout as the boxed valuetype.
+            var typeKey = new BoxedValuetypeHashtableKey(owningTypeDefinition, ownerModuleOfThunk);
+            BoxedValueType boxedTypeDefinition = _boxedValuetypeHashtable.GetOrCreateValue(typeKey);
+            return new UnboxingThunk(boxedTypeDefinition, targetMethod);


This needs to do the hashtable dance for allocation. We use object equality within the type system everywhere to compare two type system objects. This API would make it possible to have two UnboxingThunk instances that are the same thing.

MichalStrehovsky · 2017-12-01T22:12:04Z

            return thunk;
        }

+        public MethodDesc GetUnspecialUnboxingThunk(MethodDesc targetMethod, ModuleDesc ownerModuleOfThunk)


Just call it GetUnboxingThunk?

MichalStrehovsky · 2017-12-01T22:21:24Z

+            TypeDesc owningType = targetMethod.OwningType;
+            Debug.Assert(owningType.IsValueType);
+
+            var owningTypeDefinition = (MetadataType)owningType.GetTypeDefinition();


This is correctly going to the type definition, but we also need to do the inverse to specialize back before returning the result.

Consider we need an unboxing thunk for method Foo<SomeType>.Bar - this should end up creating an unboxing thunk for Boxed_Foo<T>.Bar that we then specialize to Boxed_Foo<SomeType>.Bar.

See how GetSpecialUnboxingThunk achieves that with GetTypicalMethodDefinition() and GetMethodForInstantiatedType. The special unboxing thunk only deals with generics, so it just asserts owningType != owningTypeDefinition, but in your case you'll have an if to check if we need to do that.

MichalStrehovsky · 2017-12-01T22:25:36Z

+
+                // unbox to get a pointer to the value type
+                codeStream.EmitLdArg(0);
+                codeStream.Emit(ILOpcode.unbox, emit.NewToken(_owningType));


This will throw an InvalidCastException. Owning type is not the valuetype, it's a "type with the same layout as a boxed valuetype". I would prefer we use the ldflda approach the special unbox thunk uses because it avoids the typecheck completely and codegens are much better at optimizing it.

hippiehunter · 2017-12-01T22:27:58Z

Do you have any recommendations for a specific test I can write for this? other than ToString I'm not really sure what uses unboxing stubs, and I don't think I can run ToString because of other limitations in WebAssembly right now.

MichalStrehovsky · 2017-12-01T22:31:04Z

Do you have any recommendations for a specific test I can write for this

struct Foo
{
    public int X;

    public override GetHashCode() => X;
}

Foo x = new Foo { X = 123456 };
object boxed = x;
if (boxed.GetHashCode() != x.X) Fail();

Would this work?

hippiehunter · 2017-12-06T17:56:32Z

I think I've followed all of your advice but I cant seem to get past a field of type SignatureTypeVariable getting thrown into the mix of things, causing an assert when we try to get the offset for it. If I commit what I have can you take a look?

MichalStrehovsky · 2017-12-06T18:36:35Z

If I commit what I have can you take a look?

Sure!

MichalStrehovsky · 2017-12-06T19:48:40Z

                case TypeFlags.Interface:
                case TypeFlags.Array:
                case TypeFlags.SzArray:
+                case TypeFlags.SignatureTypeVariable:


If we hit this, things are already going bad.

MichalStrehovsky · 2017-12-06T19:48:56Z

        private void ImportLoadField(int token, bool isStatic)
        {
-            FieldDesc field = (FieldDesc)_methodIL.GetObject(token);
+            FieldDesc field = ((FieldDesc)_methodIL.GetObject(token)).GetTypicalFieldDefinition();


Codegen should always look at fully instantiated things. It should never see the signature variables. Trying to load the value of field SomeGenericType<T>.SomeField should produce InvalidProgramException. Only SomeGenericType<SomeArgument>.SomeField exists. Could this be the source of trouble?

(The GetTypicalFieldDefinition doesn't look right.)

hippiehunter · 2017-12-06T21:42:23Z

Throwing seems like a good idea here, but I don't really understand how a non typical field is making it to codegen time. Is there some other issue that needs to be hunted down?

MichalStrehovsky · 2017-12-06T21:46:20Z


        private LLVMValueRef GetInstanceFieldAddress(StackEntry objectEntry, FieldDesc field)
        {
+            if (!field.IsTypicalFieldDefinition)


No, what I meant is that the call to GetTypicalFieldDefinition didn't look right.

Codegen should never operate on uninstantiated things if generics are involved. Everything should be fully instantiated. We can't compute field offsets, can't determine field sizes, etc. Uninstantiated things are useless.

Typical field definition is a field definition on the type definition. I.e. if Foo is nongeneric, Foo.Field is a typical field definition. If Foo is generic, Foo.Field is a typical field definition, Foo<Something>.Field is not a typical field definition.

The new throw doesn't look right.

I switched it to if (field.FieldType.IsSignatureVariable) as that seems to be a more direct representation of my problem, but I die later when a Internal.TypeSystem.MethodForInstantiatedType fails for obvious reasons to get a mangled name. My problem here is I don't understand why all of these open generics are suddenly flying around in codegen. How are my UnboxingStub changes causing this?

The problem is definately inside GetUnboxingStub

var owningTypeDefinition = (MetadataType)owningType.GetTypeDefinition();

if (owningType != owningTypeDefinition) { InstantiatedType boxedType = boxedTypeDefinition.MakeInstantiatedType(owningType.Instantiation); MethodDesc thunk = GetMethodForInstantiatedType(thunkDefinition, boxedType); Debug.Assert(!thunk.HasInstantiation); return thunk; }

this is where the Internal.TypeSystem.MethodForInstantiatedType is coming from. Why am i not just using the owningType directly, I think I may have just misunderstood your initial comment about this area.

You missed the InstantiateAsOpen() call in the method body generator.

The generated IL for methods on generic types is:

// [S.P.CompilerGenerated]Boxed_Enumerator<string>.get_Current_Unbox() .method instance string get_Current_Unbox() cil managed { // Code size: 12 .maxstack 12 IL_0000: ldarg.0 IL_0001: ldflda valuetype [System.Private.CoreLib]System.Collections.Generic.List`1/Enumerator<string> class Boxed_Enumerator<string>::BoxedValue IL_0006: call instance !0 valuetype [System.Private.CoreLib]System.Collections.Generic.List`1/Enumerator::get_Current() IL_000B: ret }

But it should be:

// [S.P.CompilerGenerated]Boxed_Enumerator<string>.get_Current_Unbox() .method instance string get_Current_Unbox() cil managed { // Code size: 12 .maxstack 12 IL_0000: ldarg.0 IL_0001: ldflda valuetype [System.Private.CoreLib]System.Collections.Generic.List`1/Enumerator<string> class Boxed_Enumerator<string>::BoxedValue IL_0006: call instance string valuetype [System.Private.CoreLib]System.Collections.Generic.List`1/Enumerator<string>::get_Current() IL_000B: ret }

MichalStrehovsky · 2017-12-07T18:58:03Z

+                return thunk;
+            }
+            else
+                return thunkDefinition;


Maybe add assert for !thunkDefinition.HasInstantiation. That part is a TODO (maybe add that TODO note to the same assert 4 lines above too), but I don't think we can trigger it without having a struct that implements an interface with a generic method, so it doesn't make sense to write code for it for now.

hippiehunter · 2017-12-07T23:13:54Z

this failure appears to be infrastructure related can you rerun the test on osx?

MichalStrehovsky · 2017-12-07T23:25:51Z

@dotnet-bot test OSX10.12 Debug and CoreCLR tests

MichalStrehovsky

I'll let @morganbr review the ILToWebAssemblyImporter change (I'm not paying much attention to that), but the rest looks good. Thanks!

morganbr

Looks good. Thanks, @hippiehunter!

Added real support for Unbox stubs to WebAssembly

6d2de3a

MichalStrehovsky suggested changes Dec 1, 2017

View reviewed changes

WIP addressing feedback

e2a147d

MichalStrehovsky reviewed Dec 6, 2017

View reviewed changes

throw InvalidProgramException if we try to load a non typical field

9dbd6c1

MichalStrehovsky reviewed Dec 6, 2017

View reviewed changes

added missing InstantiateAsOpen call to real method

170265f

MichalStrehovsky reviewed Dec 7, 2017

View reviewed changes

added assert for unhandled case

05747f4

MichalStrehovsky approved these changes Dec 7, 2017

View reviewed changes

morganbr approved these changes Dec 8, 2017

View reviewed changes

morganbr merged commit 8300e1f into dotnet:master Dec 8, 2017

morganbr mentioned this pull request Dec 8, 2017

Implement unbox opcode #4527

Closed

MichalStrehovsky mentioned this pull request May 25, 2018

wasm: typeof(T) fails at runtime #5842

Closed

Uh oh!

Conversation

hippiehunter commented Dec 1, 2017

Uh oh!

MichalStrehovsky commented Dec 1, 2017

Uh oh!

MichalStrehovsky commented Dec 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hippiehunter commented Dec 1, 2017

Uh oh!

MichalStrehovsky commented Dec 1, 2017

Uh oh!

hippiehunter commented Dec 6, 2017

Uh oh!

MichalStrehovsky commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hippiehunter commented Dec 6, 2017

Uh oh!

MichalStrehovsky Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hippiehunter commented Dec 7, 2017

Uh oh!

MichalStrehovsky commented Dec 7, 2017

Uh oh!

MichalStrehovsky left a comment

Choose a reason for hiding this comment

Uh oh!

morganbr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MichalStrehovsky Dec 6, 2017 •

edited

Loading