Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Provide an API to get the native module handle of the native process entry-point module #56331

Closed
jkoritzinsky opened this issue Jul 26, 2021 · 17 comments · Fixed by #57610
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Milestone

Comments

@jkoritzinsky
Copy link
Member

jkoritzinsky commented Jul 26, 2021

Background and motivation

As mentioned in #7267, there is a strong desire for enabling using the "__Internal" name in a DllImport to point to methods in the hosting executable module of the process in both CoreCLR and Mono. Mono has had this support for many years, but CoreCLR doesn't have built-in support, and we don't have a desire to provide built-in support for this "hidden" name.

However, we understand the need for a way to resolve this name to the correct handle. We propose adding an API that gets the native module handle of the entry-point module.

API Proposal

namespace System.Runtime.InteropServices
{
     public static class NativeLibrary
     {
          public static IntPtr GetMainProgramHandle();
     }
}

Alternatively, we considered allowing null as a parameter to NativeLibrary.Load(string) to match how the implementation on non-Windows is dlopen(NULL). The interop team decided that design was an abstraction leak and undesirable since the Windows implementation doesn't follow the same model. Additionally, the other NativeLibrary.Load overloads have additional behaviors and aren't thin wrappers, which makes the null option less desirable.

API Usage

NativeLibrary.SetDllImportResolver(assembly, (name, _, paths) =>
{
     if (name == "__Internal")
     {
          return NativeLibrary.GetMainProgramHandle();
     }
     return IntPtr.Zero;
});

Risks

None

@jkoritzinsky jkoritzinsky added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.InteropServices labels Jul 26, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jul 26, 2021
@jkoritzinsky
Copy link
Member Author

cc: @AaronRobinsonMSFT @elinor-fung

@AaronRobinsonMSFT
Copy link
Member

/cc @marek-safar @lambdageek

@jkoritzinsky jkoritzinsky added this to the 7.0.0 milestone Jul 26, 2021
@jkoritzinsky jkoritzinsky added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 26, 2021
@GrabYourPitchforks
Copy link
Member

Implementation question: how does this method affect the handle's reference count?

@jkoritzinsky
Copy link
Member Author

This method will cache the handle after it's been called once, so it will only increase the refcount of the handle by 1. On Windows, we'll use GetModuleHandle which doesn't increase the refcount at all.

Since the module is the process's entry-point module, we don't need to worry about releasing the handle since it is valid until process tear-down (which will release the handle anyway).

@am11
Copy link
Member

am11 commented Jul 26, 2021

Alternatively, we considered allowing null as a parameter to NativeLibrary.Load(string) to match how the implementation on non-Windows is dlopen(NULL).

Another approach is to return (IntPtr)-1 which works on Windows and Unix alike.

@lambdageek
Copy link
Member

lambdageek commented Jul 27, 2021

Alternatively, we considered allowing null as a parameter to NativeLibrary.Load(string) to match how the implementation on non-Windows is dlopen(NULL).

nit: this doesn't work on every unix. In particular on Android dlopen (NULL) doesn't work, but dlsym (RTLD_DEFAULT) does.

@bartonjs
Copy link
Member

Looks good as proposed.

namespace System.Runtime.InteropServices
{
     public static class NativeLibrary
     {
          public static IntPtr GetEntryPointModuleHandle();
     }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 10, 2021
@jkotas
Copy link
Member

jkotas commented Aug 10, 2021

GetEntryPointModuleHandle

Module is very Windows-specific name. There is nothing called Module in the existing NativeLibrary APIs. Would GetMainProgramHandle be a better cross-platform name?

I assume that the non-Windows implementation is going to return dlopen(NULL). Is that right? Note that dlopen(NULL) is a pseudo handle that does the lookup in all libraries loaded at startup, it does not correspond to a single file or module.

@jkoritzinsky
Copy link
Member Author

I didn't want to use GetMainProgramHandle since that sounds like a process handle, not a library handle that's usable with dlsym

@jkoritzinsky
Copy link
Member Author

And since the entrypoint of the program isn't a shared library, the name GetMainProgramLibraryHandle felt wrong as well.

@AaronRobinsonMSFT
Copy link
Member

Would GetMainProgramHandle be a better cross-platform name?

I don't have a strong preference here. "GetMainProgramHandle" or "GetEntryProgramHandle" are fine with me.

Note that dlopen(NULL) is a pseudo handle that does the lookup in all libraries loaded at startup, it does not correspond to a single file or module.

I was not aware of that behavior. The latest doc on dlopen makes this clear. Do we want this behavior or an actual handle to the main program?

I didn't want to use GetMainProgramHandle since that sounds like a process handle, not a library handle that's usable with dlsym

That is fair but isn't a NativeLibrary scenario what we are attempting to support. After all, I believe the point of this API is to enable "__Internal" semantics. This would imply the primary use case of the returned value is for calls to NativeLibrary.GetExport(), no?

@jkoritzinsky
Copy link
Member Author

I didn't want to use GetMainProgramHandle since that sounds like a process handle, not a library handle that's usable with dlsym

That is fair but isn't a NativeLibrary scenario what we are attempting to support. After all, I believe the point of this API is to enable "__Internal" semantics. This would imply the primary use case of the returned value is for calls to NativeLibrary.GetExport(), no?

Yes, we are trying to support a NativeLibrary scenario. My argument is that the new naming proposals imply a different scenario (process handle) than the scenario we are actually supporting and may cause confusion.

@AaronRobinsonMSFT
Copy link
Member

imply a different scenario (process handle) than the scenario we are actually supporting and may cause confusion.

Understood. I don't see this as a large concern given we can document the usage clearly and this is a niche scenario so minor confusion is okay and easily remedied. Not shutting down the conversation about renaming simply saying I don't think that will be that hard of a problem to address, but I do think that @jkotas's argument is fair - "Module" is a Windows-centric term.

Perhaps @bartonjs or @lambdageek have thoughts on naming alternatives?

@lambdageek
Copy link
Member

lambdageek commented Aug 11, 2021

Perhaps @bartonjs or @lambdageek have thoughts on naming alternatives?

(I'm not great at naming) The man pages from Apple and Linux both call the value passed to dlsym a "handle" and they mention that there are some "pseudo-handles" and in particular RTLD_DEFAULT is the "default search order pseudo handle". So...

namespace System.Runtime.InteropServices
{
     public static class NativeLibrary
     {
          public static IntPtr GetDefaultSearchOrderPseudoHandle();
     }
}

@bartonjs
Copy link
Member

Module is very Windows-specific name.

During the API review there was theorization of a companion API like IntPtr GetModuleHandle(System.Reflection.Module). Because of Assembly.Modules/Assembly.ManifestModule/etc I think that "Module" is also a .NET term. (It has much more affinity to .NET than to Windows for me, FWIW)

@jkotas
Copy link
Member

jkotas commented Aug 12, 2021

Yes, Module is also .NET metadata term. I do not think we would want to introduce API like IntPtr GetModuleHandle(System.Reflection.Module) where the returned IntPtr would be the OS library handle. The metadata module is backed by Windows OS module only on CoreCLR, only in some cases, and it changed over time. This API would be leaking internal implementation details for no good reason.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 17, 2021
@terrajobst terrajobst added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-approved API was approved in API review, it can be implemented labels Aug 25, 2021
@terrajobst
Copy link
Member

  • It was requested to rename the API from GetEntryPointModuleHandle() to GetMainProgramHandle().
  • Looks good as proposed
namespace System.Runtime.InteropServices
{
     public static class NativeLibrary
     {
          public static IntPtr GetMainProgramHandle();
     }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 25, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT added this to To do in Interop-CoreCLR 7.0 via automation Oct 18, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT moved this from To do to In progress in Interop-CoreCLR 7.0 Oct 18, 2021
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Mar 14, 2022
Interop-CoreCLR 7.0 automation moved this from In progress to Done Mar 21, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 21, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.InteropServices
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

8 participants