-
Notifications
You must be signed in to change notification settings - Fork 518
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
[runtime] Store assemblies' MVID in the generated static registrar code. #15795
Conversation
This will increase app size a little bit: the space for the MVID + 4 bytes for each assembly, but we'll be able to validate and show a helpful error message if the generated static registrar code does not match the assembly loaded at runtime. It's also a step toward per-assembly static registration (ref: xamarin#12067).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
❤️
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
if (map is null) | ||
return; | ||
|
||
for (var i = 0; i < map->assembly_count; i++) { | ||
var entry = map->assemblies [i]; | ||
var name = Marshal.PtrToStringAuto (entry.name)!; | ||
if (!Runtime.StringEquals (assembly_name, name)) |
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'm confused here a bit, why not just compare the two byte sequences?
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.
Don't need to marshal the string to do the comparison, I think?
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.
Because I couldn't think of any built-in method to do that, and I didn't feel like writing a new implementation either :)
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.
Got it, in case you want to, I think SequenceEqual can be used
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.
The main annoyance with that is that I don't have the string length, so I could either P/Invoke strlen (which isn't necessarily cheap), or write my own routine (in which case I'd just write a string-comparing routine instead of a string-length computation routine. Not a big issue, but this will happen once per assembly, and it's opt-in, so I don't think it's all that important.
try { | ||
if (!TryResolveAssembly (entry.name, out var assembly)) | ||
continue; | ||
var mvid = Marshal.PtrToStringAuto (entry.mvid)!; |
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's theoretically possible to construct the GUID from a ReadOnlySpan (although possibly not on mono?)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
📚 [PR Build] Artifacts 📚Artifacts were not provided. Pipeline on Agent XAMBOT-1098.Monterey' |
💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻✅ All tests on macOS Mac Catalina (10.15) passed. Pipeline on Agent |
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻✅ All tests on macOS M1 - Mac Big Sur (11.5) passed. Pipeline on Agent |
🚀 [CI Build] Test results 🚀Test results✅ All tests passed on VSTS: simulator tests. 🎉 All 223 tests passed 🎉 Tests counts✅ bcl: All 69 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
This will increase app size a little bit: the space for the MVID + 4 bytes for each
assembly, but we'll be able to validate and show a helpful error message if the generated
static registrar code does not match the assembly loaded at runtime.
It's also a step toward per-assembly static registration (ref: #12067).