Enhance DAC to allow getting AssemblyLoadContext for objects #37901

janvorli · 2020-06-15T12:53:35Z

This change adds support to DAC for getting AssemblyLoadContext
for a MethodTable. The returned AsseblyLoadContext is the context
into which the related type was loaded.

ghost · 2020-06-15T12:53:39Z

Tagging subscribers to this area: @tommcdon
Notify danmosemsft if you want to be subscribed.

jkotas · 2020-06-15T13:03:34Z

src/coreclr/src/debug/daccess/dacimpl.h

FYI @davmason is adding this in #37853 as well.

jkotas · 2020-06-15T13:10:52Z

src/coreclr/src/inc/sospriv.idl

Where do you get the address of the loader allocator or the binder from? Do we also need APIs to e.g. get loader allocator from MethodTable or MethodDesc?

We already have that, I have added that quite some time ago.

interface ISOSDacInterface6 : IUnknown { HRESULT GetMethodTableCollectibleData(CLRDATA_ADDRESS mt, struct DacpMethodTableCollectibleData *data); };

I also have the SOS change tested and ready to get in.

GetMethodTableCollectibleData returns LoaderAllocatorObjectHandle. How do you get from LoaderAllocatorObjectHandle to loaderAllocator ?

SOS change

It may be useful to add link to the SOS change for context.

See #11157 (comment) for the steps to get from an object to the AssemblyLoadContext. There were two steps that couldn't be done via DAC before, those are the ones I'm adding here.

As for the SOS change, I was planning to create the PR after this one gets in.

You may want to write some of the SOS code to test these new DAC APIs to test the binder code added to the DAC has been properly "DACized" (dac_cast macros, etc).

src/coreclr/src/inc/sospriv.idl

janvorli · 2020-06-15T13:23:57Z

Hmm, it seems that all Linux builds are broken due to a missing header. I am looking into it.

jkotas · 2020-06-15T16:58:30Z

src/coreclr/src/inc/dacprivate.h

Use more self-describing name, e.g. OwningAssemblyLoadContext ?

jkotas · 2020-06-15T16:59:59Z

It looks good to me otherwise. I would like to see somebody from the diagnostic team to signoff on this as well.

tommcdon · 2020-06-15T17:22:32Z

It looks good to me otherwise. I would like to see somebody from the diagnostic team to signoff on this as well.

@mikem8361 @sdmaclea can you take a look?

mikem8361 · 2020-06-15T17:26:03Z

src/coreclr/src/inc/sospriv.idl

You may want to write some of the SOS code to test these new DAC APIs to test the binder code added to the DAC has been properly "DACized" (dac_cast macros, etc).

janvorli · 2020-06-15T19:49:29Z

I am going to redo this change to add just one new method to get pointer to managed AssemblyLoadContext for a given MethodTable. The current way worked only for collectible AssemblyLoadContext and I have realized that I can make it work for both collectible and non-collectible.
My original change was trying to expose separate data structures for a subset of members of specific native structures (subset == 1 field in this case) and use it as two of multiple steps in SOS to get to the AssemblyLoadContext. But it seems that the ISOSDacInterface methods don't follow such philosophy in general, so I can just expose a method that internally does those steps on the DAC side (basically a simple pointer chain walk) and returns just the resulting AssemblyLoadContext)

janvorli · 2020-06-17T19:10:27Z

@jkotas, @mikem8361 can you please take a look again? I've made substantial changes to you'll probably want to review it from scratch. I've redone the implementation to support
dumping AssemblyLoadContext even for the cases when it is the default one or a non-collectible one. I have realized that the previous way of exposing some partial data structures is not going to work well here. So I've done it differently by adding a higher level DAC method to get AssemblyLoadContext for a MethodTable and implement all the steps needed to get from MethodTable to the ALC there. The need to access other data structures to be able to get ALC for
non-collectible types turned out into a DACization waterfall and I even had to add special handling for classes in namespaces that the DAC would not be able to handle and even update the DacTableGen to support that (symbols of classes inside namespaces are mangled in a special way).

janvorli · 2020-06-17T19:12:29Z

@jkotas as for the new interface being created here and in @davmason's change, I suppose we will want to create two new versions, otherwise we would need to merge out changes into one, right?

jkotas · 2020-06-17T19:18:37Z

create two new version

I do not think that's required as long as we ship both changes in the same release cycle.

janvorli · 2020-06-17T19:21:48Z

I can see that the Unix build is broken for the DAC build, I need to investigate it.

davmason · 2020-06-17T19:22:16Z

create two new version

I do not think that's required as long as we ship both changes in the same release cycle.

Right, as long as it's the same release we can change the interfaces.

@janvorli it should be fairly easy to merge, whoever merges last can just add their methods to the existing interface in sospriv.idl and then regenerate sospriv.h and make sure the guid definition is right in sospriv_i.cpp. There shouldn't be any other work.

jkotas · 2020-06-17T19:31:04Z

src/coreclr/src/binder/inc/assembly.hpp

This has a lot of non-DACized pointers. It is hard to see that none of them will get used by the DAC.

I have DACized only the ones that are needed for getting the ALC. It was painful enough that way.

I understand that it was painful. It is hard to see that it is correct. We prefer the type members to be fully DACized. Partially DACized types are bug farms.

mikem8361

I do like this approach better.

mikem8361 · 2020-06-17T19:29:32Z

src/coreclr/src/inc/clrprivbinderutil.h

Did you intentionally leave this commented out?

I should have removed that, I have missed it when looking through the code before commit.

jkotas · 2020-06-17T19:34:33Z

src/coreclr/src/vm/pefile.cpp

Would it be better to store the PTR_AssemblyLoadContext in the PEFile and the object handle in AssemblyLoadContext base class to avoid DACizing the whole binder machinery?

I am worried that we will keep finding holes in the binder DACization.

Would it be better to store the PTR_AssemblyLoadContext in the PEFile and the object handle in AssemblyLoadContext base class to avoid DACizing the whole binder machinery?

I will try to do it that way.

hoyosjs · 2020-06-17T22:30:29Z

src/coreclr/src/debug/daccess/request.cpp

TO_TADDR [](start = 42, length = 8)

Can you please change this to CLRDATA_ADDRESS_TO_TADDR

Sure. Btw, I am redoing it based on @jkotas feedback to simplify things considerably, so please wait with reviewing a bit.

This change adds support to DAC for getting AssemblyLoadContext for a MethodTable. The returned AsseblyLoadContext is the context into which the related type was loaded.

janvorli · 2020-06-18T14:27:35Z

@jkotas I have updated the PR based on your suggestion of storing the AssemblyLoadContext in the PEFile. It worked quite well, the only little ugly thing being the need to set the ALC for the S.P.C. separately, as the PEFile (PEAssembly) for it is created during the ICLRRuntimeHost4::Start, but the related ALC much later during ICLRRuntimeHost4::CreateAppDomainWithManager.

jkotas · 2020-06-18T14:31:14Z

src/coreclr/src/vm/pefile.cpp

 #endif
-}

+#ifndef DACCESS_COMPILE


The whole function is !DACCESS_COMPILE. This ifdef should not be needed.

Strange, I have thought I have removed it already.

jkotas · 2020-06-18T14:33:51Z

src/coreclr/src/vm/appdomain.cpp

        // Initialize the assembly binder for the default context loads for CoreCLR.
        IfFailThrow(CCoreCLRBinderHelper::DefaultBinderSetupContext(DefaultADID, &m_pTPABinderContext));
+
+        SystemDomain::SystemFile()->SetAssemblyLoadContext(m_pTPABinderContext);


Why do we need to set it explicitly? I would expect the constructor to set it for corelib too.

I've mentioned it in my comment above. the PEFile (PEAssembly) for it is created during the ICLRRuntimeHost4::Start, but the related ALC much later during ICLRRuntimeHost4::CreateAppDomainWithManager.

Maybe useful to add comment.

janvorli · 2020-06-18T19:06:30Z

There are two tests failing.

System.Runtime.Loader.Tests.RefEmitLoadContextTests.LoadRefEmitAssembly
System.Runtime.Loader.RefEmitLoadContext.Tests

It seems there is one more way how the AssemblyLoadContext would change after the PEFile creation. In Assembly::CreateDynamic, there is a call pFile->SetFallbackLoadContextBinder(pFallbackLoadContextBinder); and the m_pFallbackContext is used in the AssemblyLoadContext computation. I guess that's causing those two tests to fail. They check for AssemblyLoadContext type and get the default one instead of a specific one.

jkotas · 2020-06-18T22:05:38Z

src/coreclr/src/vm/pefile.cpp

        GetILimage()->GetLayout(PEImageLayout::LAYOUT_ANY,PEImage::LAYOUT_CREATEIFNEEDED)->Release();
 }

+AssemblyLoadContext* PEFile::LookupAssemblyLoadContext()


SetAssemblyLoadContext + LookupAssemblyLoadContext can be merged into a single method like void SetupAssemblyLoadContext() that computes it from the other PEFile state and sets it.

src/coreclr/src/vm/pefile.h

janvorli added the area-Diagnostics-coreclr label Jun 15, 2020

janvorli added this to the 5.0 milestone Jun 15, 2020

janvorli requested review from hoyosjs, jkotas and mikem8361 June 15, 2020 12:53

janvorli self-assigned this Jun 15, 2020

jkotas reviewed Jun 15, 2020

View reviewed changes

src/coreclr/src/inc/sospriv.idl Outdated Show resolved Hide resolved

jkotas reviewed Jun 15, 2020

View reviewed changes

src/coreclr/src/inc/dacprivate.h Outdated

Copy link

Member

jkotas Jun 15, 2020

Choose a reason for hiding this comment

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

Use more self-describing name, e.g. OwningAssemblyLoadContext ?

jkotas requested review from davmason and leculver June 15, 2020 16:59

mikem8361 approved these changes Jun 15, 2020

View reviewed changes

jkotas reviewed Jun 17, 2020

View reviewed changes

mikem8361 approved these changes Jun 17, 2020

View reviewed changes

jkotas reviewed Jun 17, 2020

View reviewed changes

hoyosjs reviewed Jun 17, 2020

View reviewed changes

janvorli force-pushed the add-dac-get-alc-from-obj branch from 7e2d6e3 to 2acb8c0 Compare June 18, 2020 12:51

Enhance DAC to allow getting AssemblyLoadContext for objects

0a3bfe2

This change adds support to DAC for getting AssemblyLoadContext for a MethodTable. The returned AsseblyLoadContext is the context into which the related type was loaded.

janvorli force-pushed the add-dac-get-alc-from-obj branch from 2acb8c0 to 0a3bfe2 Compare June 18, 2020 13:51

janvorli added 2 commits June 18, 2020 16:00

Change TO_TADDR to CLRDATA_ADDRESS_TO_TADDR

50a8725

Fix Unix build

6668f3b

jkotas reviewed Jun 18, 2020

View reviewed changes

Fix dynamic assemblies case and reflect PR feedback

926b06a

jkotas reviewed Jun 18, 2020

View reviewed changes

src/coreclr/src/vm/pefile.h Show resolved Hide resolved

jkotas approved these changes Jun 18, 2020

View reviewed changes

Cleanup - reflect PR feedback

c4933fb

jkotas approved these changes Jun 18, 2020

View reviewed changes

janvorli merged commit 2ddca66 into dotnet:master Jun 19, 2020

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Enhance DAC to allow getting AssemblyLoadContext for objects #37901

Enhance DAC to allow getting AssemblyLoadContext for objects #37901

Uh oh!

Conversation

janvorli commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 15, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

janvorli commented Jun 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jun 15, 2020

Uh oh!

tommcdon commented Jun 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janvorli commented Jun 15, 2020

Uh oh!

janvorli commented Jun 17, 2020

Uh oh!

janvorli commented Jun 17, 2020

Uh oh!

jkotas commented Jun 17, 2020

Uh oh!

janvorli commented Jun 17, 2020

Uh oh!

davmason commented Jun 17, 2020

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!

mikem8361 left a comment

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!

jkotas Jun 17, 2020 • 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!

janvorli commented Jun 18, 2020

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

janvorli commented Jun 15, 2020 •

edited

Loading

jkotas Jun 17, 2020 •

edited

Loading