Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[mono] Add support for UnmanagedCallersOnlyAttribute #38728
[mono] Add support for UnmanagedCallersOnlyAttribute #38728
Changes from 21 commits
e6009c9
4156eff
a8d4d57
40662c4
92ef04c
36ebfaa
376db97
ff12195
04b6da5
3ef4363
1f6d260
2579230
00ed38d
b100d40
7b00b07
6340c36
a7199ad
d6b9c56
1b5422f
6e85742
9d502c5
70075a9
025dd73
0e6f067
a2f0d94
89c8c28
ee50f3e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this 16 coming from? It looks like it's used elsewhere, but I'm not familiar enough with this code to understand why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. I'm cargo-culting the other
mono_mb_create_and_cache_full
calls in this function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, that's what I feared. @vargaz any idea? For my own learning if nothing else :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea, it probably comes from the fact that these are wrappers so the deepest stack will be calling the 'wrapped' method, so param_count + is good for max stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a class->blittable flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this, but it's not the same because of byref types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think char shouldn't be here. Doesn't look like it's supposed to be blittable. (
mono_class_setup_mono_type
doesn't set the blittable bit on it)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, good catch. Yes, it's definitely not for the same reason as string—ambiguity over converting to ANSI, UTF-8, or UTF-16.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could also have a non-blittable return type. I'd change this error slightly to reflect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this accomplishes the same thing, but wouldn't it be clearer to immediately return false on non-netcore?