Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

Conversation

@yowl
Copy link
Contributor

@yowl yowl commented May 25, 2020

This PR changes the implementation of stfld and stelem when the target type IsGCPointer and adds a test for objects rooted through a Gen 2 parent. Are there other opcodes where RhpAssignRef should be used?

Thanks @Suchiman

@jkotas
Copy link
Member

jkotas commented May 25, 2020

Are there other opcodes where RhpAssignRef should be used?

Yes: stind.ref, stobj. Also, make sure to handle value types. When the valuetype contains object references, the write barrier needs to be called for each of them.

@yowl
Copy link
Contributor Author

yowl commented May 28, 2020

I think I'm going to need to add DataLayout to LLVMSharp for this so marking as draft for now.

@yowl
Copy link
Contributor Author

yowl commented Jun 17, 2020

I have dotnet/LLVMSharp#141 which will allow this to progress I hope.

@yowl
Copy link
Contributor Author

yowl commented Jun 29, 2020

I'm having some trouble creating a failing test for assigning structs which contain object references before I actually make the fix. I have

    struct StructWithObjRefs
    {
        internal Child C1;
        internal Child C2;
    }

    class ParentOfStructWithObjRefs
    {
        internal StructWithObjRefs StructWithObjRefs;
    }

    private static ParentOfStructWithObjRefs aParentOfStructWithObjRefs;
    private static WeakReference childRef;

    private static unsafe bool TesClassInStructGC()
    {
        bool result = true;

        var parentRef = CreateParentWithStruct();
        GC.Collect();
        GC.Collect(); // parent in gen2

        StoreChildInC1();
        GC.Collect();
        if (!childRef.IsAlive)
        {
            PrintLine("Child died unexpectedly");
            result = false;
        }
        KillParentWithStruct();
        GC.Collect();
        if (childRef.IsAlive)
        {
            PrintLine("Child alive unexpectedly");
            result = false;
        }
        if (parentRef.IsAlive)
        {
            PrintLine("parent of struct Child1 alive unexpectedly");
            result = false;
        }
        return result;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    unsafe static WeakReference CreateParentWithStruct()
    {
        var parent = new ParentOfStructWithObjRefs();
        aParentOfStructWithObjRefs = parent;
        return new WeakReference(parent);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static unsafe void StoreChildInC1()
    {
        var child = new Child();
        aParentOfStructWithObjRefs.StructWithObjRefs = new StructWithObjRefs
        {
            C1 = child,
        };
        childRef = new WeakReference(child);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static void KillParentWithStruct()
    {
        aParentOfStructWithObjRefs = null;
    }

I expect the if (!childRef.IsAlive) to fail, i.e. the child stored in the struct field to have died, but it is alive for some reason. I've printed the address of the child and the parent and neither are on the stack so it seems to be rooting the Child some other way. I can add some tracing to the collection code, unless I've made a mistake in the test?

@yowl yowl force-pushed the wasm-rhpassignref branch from e684132 to bb74d7f Compare July 21, 2020 23:40
@yowl yowl force-pushed the wasm-rhpassignref branch from bb74d7f to 6fdbe26 Compare July 22, 2020 15:25
@yowl
Copy link
Contributor Author

yowl commented Jul 22, 2020

Added support for structs, which has a test, and stind.ref and stobj (don't have a test for those), both these go through ImportStoreIndirect. Unfortunately #8210 is not fixed with this change, so either I've missed something or it's elsewhere.

Thanks @Suchiman for the GC understanding.

@yowl yowl marked this pull request as ready for review July 22, 2020 15:31
}

private void CastingStore(LLVMValueRef address, StackEntry value, TypeDesc targetType, string targetName = null)
private void CastingStore(LLVMValueRef address, StackEntry value, TypeDesc targetType, bool withBarrier, string targetName = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: withBarrier -> withGCBarrier to avoid confusion with memory barrier.

}
if (!childStruct)
{
_builder.BuildStore(llvmValue, typedStoreLocation); // just copy all the fields again for simplicity, if all the fields where set using RhpAssignRef then a possible optimisation would be to skip this line
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a potential for race conditions if the LLVM codegen is used for anything multithreaded. Maybe add a comment about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, think I understand - thread1 can RhpAssignRef a struct field (after thread2 has done the same), thread1 calls BuildStore and then thread2 can get switched back in and executes BuildStore. Thread 2's field which requires the GC write barrier is then lost. Will add a comment for now.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jkotas jkotas merged commit b48a58e into dotnet:master Jul 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants