- Notifications
You must be signed in to change notification settings - Fork750
Support ARM architectures again#887
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@benoithudson, could you have a look? |
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.
Looks great! Glad to see the framework is indeed extensible.
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 is the right style; I don't have an ARM handy to test it on.
I'm assuming you're on some variant of ARM/linux?
The only thing I have is an old Raspberry Pi and a few phones, I just used godbolt to get the binaries. That's why I'd like people that had problems to test this branch ;) |
codecov-io commentedJun 20, 2019
Codecov Report
@@ Coverage Diff @@## master #887 +/- ##==========================================+ Coverage 76.86% 76.94% +0.08%========================================== Files 64 64 Lines 5909 5939 +30 Branches 974 974 ==========================================+ Hits 4542 4570 +28- Misses 1040 1042 +2 Partials 327 327
Continue to review full report at Codecov.
|
codecov-io commentedJun 20, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #887 +/- ##===========================================- Coverage 76.86% 62.92% -13.95%=========================================== Files 64 1 -63 Lines 5909 294 -5615 Branches 974 0 -974 ===========================================- Hits 4542 185 -4357+ Misses 1040 109 -931+ Partials 327 0 -327
Continue to review full report at Codecov.
|
Ummm, I think it seems that it would better if we create a dynamic library and use P/Invoke to get a function address rather than making some hard codes like these? |
That would complicate the build process quite a lot though. I'd really like to get to a single arch independent assembly in the future that can just be compiled using dotnet cli without any additional params. |
amos402 commentedJun 21, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The trick is that it's straight binary, so you only need one of these per cpu arch, not per os (no ELF vs PE distinction, no libc linking etc). Be my guest in providing a reasonable alternative. |
Oh, it is. I almost neglect it. |
Me too, which is why my patch that introduced this code was discovering the platform at runtime rather than with ifdefs. If there's some rainy days I'll see about converting more of the code. Eliminating the MONO_LINUX and MONO_OSX defines would be nice; UCS as well. |
amos402 commentedJun 21, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
By the way, the implementation for |
Probably inline the two functions. IIRC one of the calls has a CPython component, the other I didn't track down. We're getting pretty far off the original topic, I wonder if comments on this PR are the best way to gather our thoughts? |
What does this implement/fix? Explain your changes.
Adds ARM versions to the native page hack used to survive app-domain reloading.
Does this close any currently open issues?
#873