Create a profiler API that appends to the TPA list

[noahfalk]

This is a more distilled design proposal that solves issues raised in dotnet/runtime#5429. The idea is to add a new profiler API such as:

ICorProfilerInfo10::AddAssemblyPath(AppDomainID appDomainId, const WCHAR* pAssemblyPath)

which will add a path to list of assemblies that the runtime probes to resolve assembly references. This allows profilers to instrument managed code and include assembly references to a new assembly that was deployed by the profiler rather than being deployed as part of the application.

Background info to assist implementors

Probably the hardest part of the problem is creating a testable repro scenario (assuming you don't already have your own IL instrumenting profiler handy). There is an example instrumenting profiler here: https://github.com/Microsoft/clr-samples/tree/master/ProfilingAPI/ReJITEnterLeaveHooks that has most of what is necessary. However this example instruments using a CALLI instruction: https://github.com/Microsoft/clr-samples/blob/master/ProfilingAPI/ReJITEnterLeaveHooks/ILRewriter.cpp#L746

To reproduce the problem here the profiler instead needs to: 1) Include with the profiler a new managed assembly that has the implementation of some simple callback within it. Lets call this ManagedInstrumentation.dll.

     public static class ManagedCallbackFunctions
     {
         public static void OnMethodEnter(int functionId)
         {
              Console.WriteLine("Function " + functionId + " was called");
         }
     }

2) Use IMetaDataAssemblyEmit::DefineAssemblyRef to create an AssemblyRef token that refers to the ManagedInstrumentation.dll assembly. https://docs.microsoft.com/en-us/dotnet/framework/unmanaged-api/metadata/imetadataassemblyemit-defineassemblyref-method 3) Use IMetaDataEmit::DefineTypeRefByName and DefineMemberRef https://docs.microsoft.com/en-us/dotnet/framework/unmanaged-api/metadata/imetadataemit-definetyperefbyname-method https://docs.microsoft.com/en-us/dotnet/framework/unmanaged-api/metadata/imetadataemit-definememberref-method to create a MemberRef token that refers to ManagedCallbackFunctions.OnMethodEnter 4) Change the instrumentation to use an IL CALL opcode with the ManagedCallbackFunctions.OnMethodEnter token

Running an application with this profiler should fail because ManagedInstrumentation.dll can't be located.

5) To test the new API you will need to handle the AppDomainCreationFinished callback. In the handler QI for ICorProfilerInfo10 and then invoke AddTrustedAssemblyPath(domainId, path_to_ManagedInstrumentation.dll). Initially the QI will fail, but once the feature is implemented this call should succeed at modifying the assembly list. Later the compilation of instrumented methods should also succeed because the runtime will find ManagedInstrumentation.dll and load it.

To implement the feature: 1) Add the new API to https://github.com/dotnet/coreclr/blob/master/src/inc/corprof.idl 2) Add an implementation to https://github.com/dotnet/coreclr/blob/master/src/vm/proftoeeinterfaceimpl.h https://github.com/dotnet/coreclr/blob/master/src/vm/proftoeeinterfaceimpl.cpp

The implementation of the method needs to change the TPA list which is stored here: https://github.com/dotnet/coreclr/blob/32f0f9721afb584b4a14d69135bea7ddc129f755/src/binder/inc/applicationcontext.inl#L74

Navigating to that datastructure should be something like:

BaseDomain* pDomain = (BaseDomain *) appDomainId;
pDomain->GetTPABinderContext()->GetAppContext()->GetTpaList();

More background docs about CLR and profiling: https://github.com/dotnet/coreclr/tree/master/Documentation/botr https://github.com/dotnet/coreclr/blob/master/Documentation/botr/profiling.md https://github.com/dotnet/coreclr/blob/master/Documentation/botr/profilability.md

Where the TPA list normally gets set from without using a profiler: https://github.com/dotnet/coreclr/blob/ef88a92215a8f90fe0bd8b0327c16bb889902105/src/vm/corhost.cpp#L820 Inside the pProfilerNames/Values there is a property named TRUSTED_PLATFORM_ASSEMBLIES with a list of assembly paths. The runtime host constructs this list however they want (dotnet.exe assembles it from some .json configuration files like app.deps.json) and then the runtime stores it.

[bobuva]

@noahfalk, this is exactly what I need and is something I'd love to work on. What you described for defining the metadata for the ManagedInstrumentation method is something I have already done in my profiler. Not having worked on one of the dotnet repositories yet, can I just add myself to the Assignees list?

[noahfalk]

I don't see a way to assign people outside the dotnet org, but don't worry about it, consider it yours : )

[jkotas]

AddTrustedAssemblyPath

I would make it just AddAssemblyPath. The Trusted part is left-over from Silverlight that would be nice to retire eventually.

WCHAR* pAssemblyPath

It should be const WCHAR.

[noahfalk]

Agreed, that is better : ) Edited the API inline. Thanks Jan!

[bobuva]

@noahfalk I've noticed that after defining the new ICorProfilerInfo10 interface containing the one method for adding an assembly path, that the build does not find it. I found a corprof.h generated in ...\coreclr\bin\obj\Windows_NT.x64.Debug\src\inc\idls_out that contains it, but the build seems to be referencing either ...\coreclr\bin\Product\Windows_NT.x64.Debug\inc or ...\coreclr\src\pal\prebuilt\inc where ICorProfilerInfo10 is not defined.

Do I have to manually copy files over or is there something I need to run before .\build.cmd (running on Windows 10). I'm using .\build.cmd to do a complete CoreClr build. The error I get in the build is

error C2065: 'IID_ICorProfilerInfo10': undeclared identifier [C:\src\dotnet\coreclr
\bin\obj\Windows_NT.x64.Debug\src\vm\wks\cee_wks.vcxproj]

[bobuva]

Got a tiny bit forward: made sure ProfToEEInterfaceImpl derived from the new interface: class ProfToEEInterfaceImpl : public ICorProfilerInfo10. But am getting ICorProfilerInfo10 base class undefined error. Can you tell me what process you go about building when adding a new profiler level?

[noahfalk]

I just created PR dotnet/coreclr#18188 which hopefully answers the question + fixes the lacking documentation around this. Thanks for raising it and let me know if you still run into trouble.

[bobuva]

Thanks @noahfalk. Copying those files over seemed like the right thing to do.

[bobuva]

@noahfalk I've run into a bit of a roadblock converting the const WCHAR* to a LPWSTR required in the BINDER_SPACE::SimpleNameToFileNameMapEntry. See this in my fork. After readying a bit about StackSString, that seemed to be the way to go, but I'm not too clear on whether I should be using a StackSString and if I should, how it could be used in this conversion to a non-const pointer. Can you shed some light?

Thanks!

[noahfalk]

You can take a look at the code around: https://github.com/bobuva/coreclr/blob/master/src/binder/applicationcontext.cpp#L251 This is the code path that the hosting APIs use to populate this map initially, so its good pattern to follow when trying to put new assemblies into the map.

You will need to do three things which that code does: 1) Calculate an assembly simple name from the full path 2) Allocate and init a copy for both the simple name and the full path 3) Store simple name + full path copy in a MapEntry struct inside the map

The StackSString won't work out because it does the allocation for the character data on the stack. As soon as the new AddAssembly API returned the allocation would be freed but the now dangling pointer to that allocation would still be stored in the assembly map.

It would probably make sense to factor out the algorithm from 284-320 that converts full path into simple name to ensure the profiler and the hosting code are treating assembly paths exactly the same way.

[bobuva]

The plot thickens! Thanks @noahfalk.

[richlander]

Hey @bobuva ... Any progress on this? We're starting to look at which features are going to make it for .NET Core 2.2.

[bobuva]

Hi @noahfalk, @richlander,

I haven't made a lot of progress in the past month mainly because of my work responsibilities. I did create this plan for myself:

Tasks

• [X] Fork coreclr and build locally
• [X] Add AddAssemblyPath API to new ICorProfilerInfo10 interface.
• [ ] Factor out common code shared with ApplicationContext class.
• [ ] Tests
• [ ] Submit PR
• [ ] Respond to PR comments

As you can see, based on Noah's suggestion, I'm working on the factoring out of shared code; have looked at it but have not begun that yet. When would the feature need to be ready to get into 2.2?

[noahfalk]

@bobuva - just wanted to mention https://github.com/dotnet/coreclr/issues/3894#issuecomment-421094674 here in case you hadn't seen it. If the startup hook would solve the issue for you equally well you might decide you'd prefer to use that vs. invest here.

[bobuva]

@noahfalk I had heard about this -- thank you, yes it is probably a better way to go although the timing may be tricky. Thanks!

[tommcdon]

@noahfalk moving this issue out to "future", feel free to pull this back to 3.0 if we believe the PR will be ready in the 3.0 timeframe

[bobuva]

@bobuva - just wanted to mention #3894 (comment) here in case you hadn't seen it. If the startup hook would solve the issue for you equally well you might decide you'd prefer to use that vs. invest here.

@noahfalk Is this the same thing as the startup hook you referred to above?

[noahfalk]

No, the one you linked to there is a coding pattern used by developers for ASP.NET apps and code is running within the Main method. The one I was referring to above (https://github.com/dotnet/core-setup/blob/master/Documentation/design-docs/host-startup-hook.md) is an environment variable supported by the underlying .net core runtime and it can be used to execute code before Main(). It does not require any coordination with the app's developer as it can be injected by someone who is in control of the environment where the app runs.