-
Notifications
You must be signed in to change notification settings - Fork 197
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
Add basic support for compiling to ARM32 #1294
Conversation
What do you need the rare flags for? |
@@ -647,7 +647,9 @@ inline uint8_t * PalNtCurrentTeb() | |||
// conditional compilation (upto and including defining an export of functionality that isn't a supported | |||
// intrinsic on that platform). | |||
// | |||
|
|||
#if defined(HOST_ARM) | |||
#define __cdecl |
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 do the other architectures (x64 or arm64) get the cdecl from?
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 assume this is MSVC specific calling convention, and probably this code was never compiled on non-Windows.
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 assume that mean I cannot find anything which make me think otherwise. I looking for gcc and clang but seems to be it's only MSVC specific
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.
Right, this file is included for Linux x64 or arm64 as well. Why is clang not complaining about this cdecl on existing Linux targets?
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 what I still cannot get where this is included.
I found following 3 files where __cdecl is redefined if needed
src\coreclr\pal\inc\pal_mstypes.h
src\coreclr\inc\palclr.h
src\coreclr\inc\coredistools.h
Last one seems to be not connected to building Runtime/Full, but other two can be somehow included. I try to track down
I will continue hunt down, since on my Linux arm64 with command ./build.sh nativeaot -a arm64 -rc Debug -lc Release
I pass that point, so somewhere I should change macro conditions.
Side-node: Not sure if this is expected,
error MSB3030: Could not copy the file "~/runtimelab/artifacts/bin/coreclr/Linux.arm64.Debug/libjitinterface_arm64.so" because it was not found. [/~/runtimelab/src/coreclr/tools/aot/ILCompiler/ILCompiler.csproj]```
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.
Could not copy the file "~/runtimelab/artifacts/bin/coreclr/Linux.arm64.Debug/libjitinterface_arm64.so"
It is not expected. We do official arm64 builds via cross-compilation on Linux x64 so it may explain why we do not see 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.
Things become more interesting. It seems that I know the reason why ARM not working.
Difference in compilation flags give me hint to the problem.
ARM
clang test.cpp -lstdc++ -fms-extensions -march=armv7-a
Literally anything else
clang test.cpp -lstdc++ -fms-extensions
Irregardless of -march
presence it produce warning on ARM, and only way to suppress that is to undefine __cdecl
.
So options are following:
- Should I place __cdecl augmentation in the
src/coreclr/nativeaot/Runtime/PalRedhawk.h
orsrc/coreclr/nativeaot/Runtime/PalRedhawkCommon.h
- Or maybe I can include
src\coreclr\pal\inc\pal_mstypes.h
orsrc\coreclr\inc\palclr.h
inPalRedhawkXXX.h
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.
Should I place __cdecl augmentation in the src/coreclr/nativeaot/Runtime/PalRedhawk.h or src/coreclr/nativeaot/Runtime/PalRedhawkCommon.h
Yes, that sounds good.
This is most puzzling thing for me. I notice that this is missing between CoreRT and NativeAOT. Actual copy start after I hit compilation errors here runtimelab/src/coreclr/nativeaot/Runtime/RuntimeInstance.cpp Lines 449 to 459 in e8ca007
This was only place where RareFlags was used. Sorry that I forgot to mention that in the original issue description. Whole PR is more question, then actual work. |
When building using on x64 Linux with command line error produced
|
src/coreclr/inc/regdisp.h
Outdated
@@ -155,7 +155,7 @@ inline TADDR GetRegdisplayStackMark(REGDISPLAY *display) { | |||
|
|||
#elif defined(TARGET_64BIT) | |||
|
|||
#if defined(TARGET_ARM64) | |||
#if defined(TARGET_ARM64) || defined(TARGET_ARM) |
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 does not look right. TARGET_64BIT
above should not be defined for TARGET_ARM
. 32-bit ARM has very different volatile context pointers.
We should rewrite this in C#. The extra indirection is not needed anymore, so this should just directly return the function pointer. |
@jkotas is this related to this whole function, or to only offending part? |
The whole 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.
This looks good to me as is. Is it fine to merge? (The PR is still marked [DRAFT].)
This is still draft. I have other recommendations replace with C# and most
likely this is requirement. That’s another can of worm for me. I can show
want I have because I only barely understand what’s going on yet.
…On Fri, Jul 9, 2021 at 06:39 Jan Kotas ***@***.***> wrote:
***@***.**** approved this pull request.
This looks good to me as is. Is it fine to merge? (The PR is still marked
[DRAFT].)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1294 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABAPKN6PWLSPNAOK4JQJXALTWZANLANCNFSM47YFKUEQ>
.
|
It is fine to do that in separate PR. This PR is a good incremental step in its current state. I would be happy to merge it if you drop "DRAFT" from the title. |
This is not yet compiling. Given that it was broken, I would not mind, but maybe not what you expect. |
But I'm more then happy with that, since I working on update libunwind, so I let it hang until you merge that PR then. And really rework of |
Thank you! |
When I copy over that file, I do not realize that these 2 functions was already declared in GcProbe.asm and this file needed only for build system. So far left following - Produce libObjWriter.so for ARM - Rewrite RhGetRuntimeHelperForType in C# as per this discussion dotnet#1294 (comment)
This is what I do to make it compile on Raspbian on my Raspberry Pi 3B+
I try to resurrect dotnet/corert@d9847af in changes to regdisp.h but seems to be I do wrong thing here, and I should resurrect definitions from there.
I would like to understand is RareFlags are still valid way, and if not what's the proper way.
See #833