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

[MonoAPI] Split type and function headers, add MONO_API_FUNCTION macro #65446

Merged
merged 14 commits into from
Feb 24, 2022

Conversation

lambdageek
Copy link
Member

The idea is that the function header can be included multiple times with
different definitions of MONO_API_FUNCTION in order to make it easier to
re-used the definitions for embedding the runtime in late-binding scenarios

Contributes to #64456

@lambdageek
Copy link
Member Author

@joncham @vargaz Want to get your feedback before I do metadata/ and mini/.

@joncham I think this will work for Unity-Technologies#4

@lambdageek
Copy link
Member Author

This one is ready for review, @vargaz

Went ahead and changed the metadata and jit headers too. Also renamed the jit header directory - this will eliminate the difference between how to include jit.h in-tree vs in an embedding scenario, which will simplify the test PR. Updated the runtime to #include <mono/jit/jit.h>.

@joncham
Copy link
Contributor

joncham commented Feb 18, 2022

@lambdageek one thing I just hit in my integration is calling convention. On 32-bit windows CoreCLR is built specifying STDCALL as the calling convention. This required me to decorate all my exported Mono functions with __cdecl. I think the current macro approach will let me do same, just FYI.

@lambdageek
Copy link
Member Author

@lambdageek one thing I just hit in my integration is calling convention. On 32-bit windows CoreCLR is built specifying STDCALL as the calling convention. This required me to decorate all my exported Mono functions with __cdecl. I think the current macro approach will let me do same, just FYI.

@joncham, just so I'm clear, you'd do something like this, right?

extern "C" {
#define MONO_API_FUNCTION(ret,name,args) typedef ret (__cdecl * name##_type) args;
#include <mono/metadata/details/object-functions.h>
#undef MONO_API_FUNCTION

#define MONO_API_FUNCTION(ret,name,args) static name##_type name;
#include <mono/metadata/details/object-functions.h>
#undef MONO_API_FUNCTION

static void
init_functions()
{
  HMODULE hModule = ...
  #define MONO_API_FUNCTION(ret,name,args) name = (name ##_type)GetProcAddress(hModule, #name );
  #include <mono/metadata/details/object-functions.h>
  #undef MONO_API_FUNCTION
}

@joncham
Copy link
Contributor

joncham commented Feb 22, 2022

@joncham, just so I'm clear, you'd do something like this, right?

Correct.

*
* Private unstable APIs.
*
* WARNING: The declarations and behavior of functions in this header are NOT STABLE and can be modified or removed at
Copy link
Member

Choose a reason for hiding this comment

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

nit: it would be nice to retain this warning in mono-private-unstable-types.h and mono-private-unstable-functions.h too.

install(TARGETS monoapi_mini PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/mono-2.0/mono/jit)
install(DIRECTORY mono/utils/ DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/mono-2.0/mono/utils)
install(DIRECTORY mono/metadata/ DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/mono-2.0/mono/metadata)
install(DIRECTORY mono/jit/ DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/mono-2.0/mono/jit)
Copy link
Member

@akoeplinger akoeplinger Feb 22, 2022

Choose a reason for hiding this comment

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

this will inadvertently install other files we put into these directories, e.g. README.md, right?
could we instead split jit_public_headers_base etc. into the normal and details/ .h files and install them separately here?

Copy link
Member Author

Choose a reason for hiding this comment

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

this will inadvertently install other files we put into these directories, e.g. README.md, right? could we instead split jit_public_headers_base etc. into the normal and details/ .h files and install them separately here?

Yea I think so. I would just use install(FILES) then, I think, right? I don't really know what i'm doing with cmake install rules.

Copy link
Member

Choose a reason for hiding this comment

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

Yes that sounds good.

Copy link
Member

Choose a reason for hiding this comment

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

We normally don't use install(DIRECTORY, @lambdageek. Elsewhere, we explicitly capture all the needed files in a variable, then use: install(FILES ${that_var} DESTINATION path/to/some/dir).

@akoeplinger
Copy link
Member

We should also update the CODEOWNERS file.

The idea is that the function header can be included multiple times with
different definitions of MONO_API_FUNCTION in order to make it easier to
re-used the definitions for embedding the runtime in late-binding scenarios
install(TARGETS) flattens subdirectories  (so all the details/ headers ended up
in the respective parent directory)
One complication here is that the directory is called "jit" outside the
runtime, but "mini", inside.
To match how embedders see the tree.

Update the runtime to include <mono/jit/jit.h> instead of <mono/mini/jit.h>.

No change in public API
@lambdageek
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lambdageek
Copy link
Member Author

lambdageek commented Feb 23, 2022

I'm going to do a follow-up PR with a make one more slight tweak.

Instead of defining functions like this:

MONO_API_FUNCTION(MONO_API MONO_RT_EXTERNAL_ONLY returnType, mono_function_name, (MonoObject *arg1, MonoObject* arg2))

I'm going to pull the MONO_API part into the default definition of MONO_API_FUNCTION, and probably MONO_RT_EXTERNAL_ONLY into a new modifiers argument:

MONO_API_FUNCTION(MONO_RT_EXTERNAL_ONLY, returnType, mono_function_name, (MonoObject* arg1, MonoObject *arg2))

where we define the macro as:

#define MONO_API_FUNCTION(modifiers, ret, name, args) MONO_API modifiers ret name args;

The issue is that MONO_API in C++ builds expands to extern "C", which is in an illegal place if you're using a MONO_API_FUNCTION to typedef a function pointer type.

I'm less sure about the modifiers part - currently MONO_RT_EXTERNAL_ONLY is the only one that we have and it expands to the empty string outside the runtime, so maybe it's not worth pulling it out. But it doesn't really belong with the return type, either. And there might be some other attributes that we might want in the future (e.g. for deprecation warnings) that different MONO_API_FUNCTION definitions might want to strip off.

FYI /cc @joncham

@lambdageek
Copy link
Member Author

I ended up redefining MONO_API_FUNCTION on this PR. still with 3 arguments. 4 arguments where the 1st argument is mostly empty looked ugly (e.g. MONO_API_FUNCTION(,MonoObject *, mono_some_function, (MonoObject *arg1))) and MONO_RT_EXTERNAL_ONLY expands to empty outside the runtime build, so it didn't seem worth it just for that.

@lambdageek
Copy link
Member Author

"CoreCLR Product Build windows arm64 " (checked and release) build failures are #65624 (comment)

@lambdageek lambdageek merged commit 858221c into dotnet:main Feb 24, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants