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

CoreCLR building on ARM #1210

Merged
merged 3 commits into from Jul 24, 2015

Conversation

Projects
None yet
@benpye
Contributor

benpye commented Jul 7, 2015

This PR allows for CoreCLR to be built on ARM. It has been tested on Ubuntu 14.04 on a Raspberry Pi 2 but should work on any reasonably similar Linux distribution with ARMv7, VFPv3 and Thumb 2 support. All PAL tests pass and I believe this would be in a suitable state for CI as it builds cleanly.

The JIT is as of this PR non functional, however, to enable bring up of other components it is possible to use the legacy JIT. This can be enabled by modifying the src/jit/CMakeLists.txt file, so that the following is under elseif(CLR_CMAKE_PLATFORM_ARCH_ARM) (line 74).

set( ARCH_SOURCES
  targetarm.cpp
  unwindarm.cpp
  emitarm.cpp
  codegenlegacy.cpp
  registerfp.cpp
)
add_definitions(-DLEGACY_BACKEND) 

With the legacy JIT backend, this PR actually gets the ARM port to the point of running a simple hello world application, though there is still issues in stack unwinding so GC and exception handling at the very least are still non functional. I have gotten further in another branch, however I am currently blocked by a Clang bug (see https://llvm.org/bugs/show_bug.cgi?id=24146 ).

The sos plugin for lldb will build on ARM however lldb itself will not work on ARM, instead segfaulting when trying to execute corerun. This is an lldb bug and when lldb is fixed, it should be possible to use sos to debug CoreCLR on ARM, though there may be issues.

set(CLR_CMAKE_PLATFORM_UNIX_TARGET_AMD64 1)
if(CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64)
set(CLR_CMAKE_PLATFORM_UNIX_TARGET_AMD64 1)
elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL armv7l)

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

armv7l is little-endian armv7, but you seem to be targeting hard-float ABI, which is (generally) known as armv7hl. Perhaps this isn't consistent across kernels/cmakes? Can you add armv7hl as well?

@kangaroo

kangaroo Jul 7, 2015

Contributor

armv7l is little-endian armv7, but you seem to be targeting hard-float ABI, which is (generally) known as armv7hl. Perhaps this isn't consistent across kernels/cmakes? Can you add armv7hl as well?

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

Can add it, but I haven't seen any example of having uname output armv7hl?

@benpye

benpye Jul 7, 2015

Contributor

Can add it, but I haven't seen any example of having uname output armv7hl?

This comment has been minimized.

@josteink

josteink Jul 24, 2015

Member

Just to add some real world data. Raspbian (which is hard-float) running on a classic Raspberry pi (1) gives the following output:

$ uname -a
Linux Applepi 3.18.7+ #755 PREEMPT Thu Feb 12 17:14:31 GMT 2015 armv6l GNU/Linux

Note there is no armv6hl in the output. Not sure if this is enough data to base a generalization on, but at least it's data :)

@josteink

josteink Jul 24, 2015

Member

Just to add some real world data. Raspbian (which is hard-float) running on a classic Raspberry pi (1) gives the following output:

$ uname -a
Linux Applepi 3.18.7+ #755 PREEMPT Thu Feb 12 17:14:31 GMT 2015 armv6l GNU/Linux

Note there is no armv6hl in the output. Not sure if this is enough data to base a generalization on, but at least it's data :)

This comment has been minimized.

@benpye

benpye Jul 24, 2015

Contributor

Unfortunately ARMv6 is going to be harder to support. So currently only the
RPi 2 will work at this time.

On Fri, 24 Jul 2015 09:27 Jostein Kjønigsen notifications@github.com
wrote:

In CMakeLists.txt
#1210 (comment):

@@ -16,7 +16,16 @@ set(VM_DIR ${CMAKE_CURRENT_SOURCE_DIR}/src/vm)

if(CMAKE_SYSTEM_NAME STREQUAL Linux)
set(CLR_CMAKE_PLATFORM_UNIX 1)

  • set(CLR_CMAKE_PLATFORM_UNIX_TARGET_AMD64 1)
  • if(CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64)
  •    set(CLR_CMAKE_PLATFORM_UNIX_TARGET_AMD64 1)
    
  • elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL armv7l)

Just to add some real world data. Raspbian (which is hard-float) running
on a classic Raspberry pi (1) gives the following output:

$ uname -a
Linux Applepi 3.18.7+ #755 PREEMPT Thu Feb 12 17:14:31 GMT 2015 armv6l GNU/Linux

Note there is no armv6hl in the output. Not sure if this is enough data
to base a generalization on, but at least it's data :)


Reply to this email directly or view it on GitHub
https://github.com/dotnet/coreclr/pull/1210/files#r35404087.

@benpye

benpye Jul 24, 2015

Contributor

Unfortunately ARMv6 is going to be harder to support. So currently only the
RPi 2 will work at this time.

On Fri, 24 Jul 2015 09:27 Jostein Kjønigsen notifications@github.com
wrote:

In CMakeLists.txt
#1210 (comment):

@@ -16,7 +16,16 @@ set(VM_DIR ${CMAKE_CURRENT_SOURCE_DIR}/src/vm)

if(CMAKE_SYSTEM_NAME STREQUAL Linux)
set(CLR_CMAKE_PLATFORM_UNIX 1)

  • set(CLR_CMAKE_PLATFORM_UNIX_TARGET_AMD64 1)
  • if(CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64)
  •    set(CLR_CMAKE_PLATFORM_UNIX_TARGET_AMD64 1)
    
  • elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL armv7l)

Just to add some real world data. Raspbian (which is hard-float) running
on a classic Raspberry pi (1) gives the following output:

$ uname -a
Linux Applepi 3.18.7+ #755 PREEMPT Thu Feb 12 17:14:31 GMT 2015 armv6l GNU/Linux

Note there is no armv6hl in the output. Not sure if this is enough data
to base a generalization on, but at least it's data :)


Reply to this email directly or view it on GitHub
https://github.com/dotnet/coreclr/pull/1210/files#r35404087.

This comment has been minimized.

@josteink

josteink Jul 24, 2015

Member

I have no problems with that. Just supporting your notion that hard-float (as brought up by @kangaroo) often doesn't show in uname -a.

@josteink

josteink Jul 24, 2015

Member

I have no problems with that. Just supporting your notion that hard-float (as brought up by @kangaroo) often doesn't show in uname -a.

This comment has been minimized.

@Tragetaschen

Tragetaschen Jul 24, 2015

Contributor

Just to add to the confusion :)

I'm targeting a Freescale i.MX 6DualLite (with VFPv3) using a hard-float toolchain from the Yocto project.

$ uname -a
Linux f500-kai 4.0.0-00146-g52ee60d-dirty #45 SMP Thu Jul 23 16:20:44 CEST 2015 armv7l GNU/Linux
@Tragetaschen

Tragetaschen Jul 24, 2015

Contributor

Just to add to the confusion :)

I'm targeting a Freescale i.MX 6DualLite (with VFPv3) using a hard-float toolchain from the Yocto project.

$ uname -a
Linux f500-kai 4.0.0-00146-g52ee60d-dirty #45 SMP Thu Jul 23 16:20:44 CEST 2015 armv7l GNU/Linux
set(CLR_CMAKE_PLATFORM_UNIX_TARGET_ARM 1)
# Because we don't use CMAKE_C_COMPILER/CMAKE_CXX_COMPILER to use clang
# we have to set the triple by adding a compiler argument
add_compile_options(-target armv7-linux-gnueabihf)

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

See above, armv7l does not guarantee HF. We may want to turn this into a configure time check.

@kangaroo

kangaroo Jul 7, 2015

Contributor

See above, armv7l does not guarantee HF. We may want to turn this into a configure time check.

# we have to set the triple by adding a compiler argument
add_compile_options(-target armv7-linux-gnueabihf)
add_compile_options(-mthumb)
add_compile_options(-mfpu=vfpv3)

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Same, v7 does not guarantee vfp, or vfpv3, we probably want to probe for this, and eventually support x-compile overrides.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Same, v7 does not guarantee vfp, or vfpv3, we probably want to probe for this, and eventually support x-compile overrides.

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

I was under the impression that these were the assumptions made by CoreCLR at least on Windows, correct me if I am wrong. And these are also the base requirements of Ubuntu for ARM if we are keeping that as our target.

@benpye

benpye Jul 7, 2015

Contributor

I was under the impression that these were the assumptions made by CoreCLR at least on Windows, correct me if I am wrong. And these are also the base requirements of Ubuntu for ARM if we are keeping that as our target.

This comment has been minimized.

@kangaroo

kangaroo Jul 15, 2015

Contributor

They are the assumptions made on windows, but we can compile time confirm them on !windows.

@kangaroo

kangaroo Jul 15, 2015

Contributor

They are the assumptions made on windows, but we can compile time confirm them on !windows.

This comment has been minimized.

@benpye

benpye Jul 15, 2015

Contributor

How would you suggest this is done, only examples I can see are to get the output of /proc/cpuinfo in CMake and see if features are available. Also, whilst VFPv3 may not be guaranteed, at least in Cortex cores, it's included in all but the A5 and A9 where it is supposedly optional, but I haven't seen an example lacking it.

@benpye

benpye Jul 15, 2015

Contributor

How would you suggest this is done, only examples I can see are to get the output of /proc/cpuinfo in CMake and see if features are available. Also, whilst VFPv3 may not be guaranteed, at least in Cortex cores, it's included in all but the A5 and A9 where it is supposedly optional, but I haven't seen an example lacking it.

This comment has been minimized.

@kangaroo

kangaroo Jul 15, 2015

Contributor

We could have a CHECK_CXX_SOURCE_COMPILES check with vfp3 assembler? That would at least guarantee the toolchain being used supports what we need. Thoughts?

@kangaroo

kangaroo Jul 15, 2015

Contributor

We could have a CHECK_CXX_SOURCE_COMPILES check with vfp3 assembler? That would at least guarantee the toolchain being used supports what we need. Thoughts?

Show outdated Hide outdated CMakeLists.txt
@@ -328,6 +345,9 @@ add_compile_options(-Wno-invalid-offsetof)
# may not generate the same object layout as MSVC.
add_compile_options(-Wno-incompatible-ms-struct)
#This causes some issues in the JIT
add_compile_options(-Wno-unused-value)

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Please guard this in an ARM specific block, if its arm specific.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Please guard this in an ARM specific block, if its arm specific.

endif(IS_64BIT_BUILD)
add_definitions(-DFEATURE_PAL)

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Lets not add this conditionally across the board. Please expand the above logic block to handle the ARM/32 case. Keep in mind ARM64 is coming as well.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Lets not add this conditionally across the board. Please expand the above logic block to handle the ARM/32 case. Keep in mind ARM64 is coming as well.

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

Aren't we still going to use PAL where CLR_CMAKE_PLATFORM_UNIX is defined?

@benpye

benpye Jul 7, 2015

Contributor

Aren't we still going to use PAL where CLR_CMAKE_PLATFORM_UNIX is defined?

This comment has been minimized.

@janvorli

janvorli Jul 8, 2015

Member

We are. Anything non-windows will use the PAL. So I think it is fine to define it across the board.

@janvorli

janvorli Jul 8, 2015

Member

We are. Anything non-windows will use the PAL. So I think it is fine to define it across the board.

Show outdated Hide outdated CMakeLists.txt
add_definitions(-D_ARM_)
add_definitions(-DARM)
else ()
# TODO: Support this

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Is there an else here? If not lets #error for now.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Is there an else here? If not lets #error for now.

Show outdated Hide outdated src/CMakeLists.txt
include_directories("debug/inc/amd64")
if(CMAKE_SYSTEM_PROCESSOR STREQUAL x86_64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL amd64 OR CMAKE_SYSTEM_PROCESSOR STREQUAL AMD64)
include_directories("debug/inc/amd64")
elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL armv7l)

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Ditto above on armv7l comments.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Ditto above on armv7l comments.

Show outdated Hide outdated src/CMakeLists.txt
elseif(CMAKE_SYSTEM_PROCESSOR STREQUAL armv7l)
include_directories("debug/inc/arm")
else()
# IMPLEMENT THIS

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

No IMPLEMENT THIS please, #error if its not a use case yet.

@kangaroo

kangaroo Jul 7, 2015

Contributor

No IMPLEMENT THIS please, #error if its not a use case yet.

Show outdated Hide outdated src/CMakeLists.txt
@@ -42,5 +49,5 @@ if(WIN32)
endif(WIN32)
if(CLR_CMAKE_PLATFORM_UNIX)
add_subdirectory(palrt)
add_subdirectory(palrt)

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Whitespace.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Whitespace.

dtcontext->R8 = GetRegister(frame, "r8");
dtcontext->R9 = GetRegister(frame, "r9");
dtcontext->R10 = GetRegister(frame, "r10");
dtcontext->R11 = GetRegister(frame, "r11");

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

r12?

@kangaroo

kangaroo Jul 7, 2015

Contributor

r12?

This comment has been minimized.

@benpye

benpye Jul 10, 2015

Contributor

Added in later commit.

@benpye

benpye Jul 10, 2015

Contributor

Added in later commit.

Show outdated Hide outdated src/binder/CMakeLists.txt
include_directories(BEFORE "../vm/amd64")
elseif(CLR_CMAKE_PLATFORM_ARCH_ARM)
include_directories(BEFORE "../vm/arm")
endif()

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

lets future proof arm64 with an else #error for now

@kangaroo

kangaroo Jul 7, 2015

Contributor

lets future proof arm64 with an else #error for now

Show outdated Hide outdated src/classlibnative/CMakeLists.txt
if(CLR_CMAKE_PLATFORM_ARCH_AMD64)
include_directories(BEFORE "../vm/amd64")
elseif(CLR_CMAKE_PLATFORM_ARCH_ARM)
include_directories(BEFORE "../vm/arm")

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

lets future proof arm64 with an else #error for now

@kangaroo

kangaroo Jul 7, 2015

Contributor

lets future proof arm64 with an else #error for now

Show outdated Hide outdated src/classlibnative/CMakeLists.txt
add_definitions(-DDBG_TARGET_32BIT=1)
add_definitions(-DDBG_TARGET_ARM=1)
add_definitions(-DDBG_TARGET_WIN32=1)
endif()

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

lets future proof arm64 with an else #error for now

@kangaroo

kangaroo Jul 7, 2015

Contributor

lets future proof arm64 with an else #error for now

Show outdated Hide outdated src/corefx/System.Security.Cryptography.Native/CMakeLists.txt
add_definitions(-DBIT64=1)
elseif(CLR_CMAKE_PLATFORM_ARCH_ARM)
add_definitions(-DBIT32=1)
endif()

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

going to stop repeating: "lets future proof arm64 with an else #error for now", ditto all the way down.

@kangaroo

kangaroo Jul 7, 2015

Contributor

going to stop repeating: "lets future proof arm64 with an else #error for now", ditto all the way down.

Show outdated Hide outdated src/debug/daccess/CMakeLists.txt
list(APPEND DACCESS_SOURCES
i386/primitives.cpp
arm/primitives.cpp

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

This seems to inadvertently remove the (basic blocks) of i386 build support. Can we keep that intact please?

@kangaroo

kangaroo Jul 7, 2015

Contributor

This seems to inadvertently remove the (basic blocks) of i386 build support. Can we keep that intact please?

@@ -8,7 +8,6 @@
//
//*****************************************************************************
#include "stdafx.h"

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

Equivalent files for AMD64 do not have this include, additionally it errors, appears to be missing.

@benpye

benpye Jul 7, 2015

Contributor

Equivalent files for AMD64 do not have this include, additionally it errors, appears to be missing.

@@ -5,6 +5,4 @@
//
#include "stdafx.h"

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

Same as above.

@benpye

benpye Jul 7, 2015

Contributor

Same as above.

@@ -29,7 +38,6 @@ set(CORDBEE_SOURCES_WKS
shared.cpp
frameinfo.cpp
${ARCH_SOURCES_DIR}/primitives.cpp
${ARCH_SOURCES_DIR}/debuggerregdisplayhelper.cpp
)

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

The file is AMD64/i386 specific, it's added back further down.

@benpye

benpye Jul 7, 2015

Contributor

The file is AMD64/i386 specific, it's added back further down.

Show outdated Hide outdated src/debug/ee/CMakeLists.txt
else ()
list(APPEND CORDBEE_SOURCES_WKS i386/x86walker.cpp)
endif (IS_64BIT_BUILD EQUAL 1)
elseif(CLR_CMAKE_PLATFORM_ARCH_ARM)

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Same as above, please don't nuke the primitive i386 support.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Same as above, please don't nuke the primitive i386 support.

Show outdated Hide outdated src/debug/ee/arm/dbghelpers.S
// r0 : pointer to DebuggerEval object
//
NESTED_ENTRY FuncEvalHijack,_TEXT,FuncEvalHijackPersonalityRoutine

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

(nit) Whitespace inbetween the ,'s please.

@kangaroo

kangaroo Jul 7, 2015

Contributor

(nit) Whitespace inbetween the ,'s please.

Show outdated Hide outdated src/debug/ee/arm/dbghelpers.S
// r3 : void* pdata
//
NESTED_ENTRY ExceptionHijack,_TEXT,ExceptionHijackPersonalityRoutine

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

(nit) Spaces between the ,'s please.

@kangaroo

kangaroo Jul 7, 2015

Contributor

(nit) Spaces between the ,'s please.

Show outdated Hide outdated src/inc/clrnt.h
// This function returns the length of a function using the new unwind info on arm.
// Taken from minkernel\ntos\rtl\arm\ntrtlarm.h.
FORCEINLINE

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

Show outdated Hide outdated src/inc/clrnt.h
return FunctionEntry->BeginAddress + 2 * FunctionLength;
}
);

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

Show outdated Hide outdated src/jit/CMakeLists.txt
)
add_definitions(-DLEGACY_BACKEND)
else()
# IMPLEMENT THIS

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Ditto, error please

@kangaroo

kangaroo Jul 7, 2015

Contributor

Ditto, error please

@@ -778,7 +778,7 @@ void CodeGen::genCodeForBBlist()
}
}
}
#endif _TARGET_AMD64_
#endif //_TARGET_AMD64_

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Please dont include style changes in functional diffs.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Please dont include style changes in functional diffs.

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

What's the best solution here? Revert and suppress the warning (this warning is enabled in all cases currently), or to commit these fixes, it's as this code was never built on Linux so it does somewhat come under the get it building.

@benpye

benpye Jul 7, 2015

Contributor

What's the best solution here? Revert and suppress the warning (this warning is enabled in all cases currently), or to commit these fixes, it's as this code was never built on Linux so it does somewhat come under the get it building.

This comment has been minimized.

@jkotas

jkotas Jul 7, 2015

Member

I would keep the fixes for the warnings.

@jkotas

jkotas Jul 7, 2015

Member

I would keep the fixes for the warnings.

@@ -1010,7 +1010,7 @@ void CodeGen::instGen_Set_Reg_To_Imm(emitAttr size,
getEmitter()->emitIns_R_AI(INS_lea, EA_PTR_DSP_RELOC, reg, imm);
}
else
#endif _TARGET_AMD64_
#endif // _TARGET_AMD64_

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Please dont include style changes in functional diffs.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Please dont include style changes in functional diffs.

@@ -703,4 +703,4 @@ protected :
#endif // LEGACY_BACKEND
#endif _CODEGENCLASSIC_H_
#endif // _CODEGENCLASSIC_H_

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Please dont include style changes in functional diffs.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Please dont include style changes in functional diffs.

Show outdated Hide outdated src/jit/codegencommon.cpp
@@ -10824,6 +10824,9 @@ CORINFO_CLASS_HANDLE Compiler::GetHfaClassHandle(GenTreePtr tree)
case GT_ASG:
assert(tree->gtOp.gtOp1->gtOper == GT_LCL_VAR || tree->gtOp.gtOp1->gtOper == GT_LCL_FLD);
return GetHfaClassHandle(tree->gtOp.gtOp1);
default:
break;

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why was this being hit? Tag @CarolEidt

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why was this being hit? Tag @CarolEidt

This comment has been minimized.

@CarolEidt

CarolEidt Jul 7, 2015

Member

I don't believe it was being hit; I think that this was added because of a code review tool that required default clauses for case statements.

@CarolEidt

CarolEidt Jul 7, 2015

Member

I don't believe it was being hit; I think that this was added because of a code review tool that required default clauses for case statements.

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

That's correct, clang throws a warning on cases for enums when they lack cases for all values. I can disable the warning and revert these changes if you'd like to keep this purely functional, as above with the commenting after #ifdef.

@benpye

benpye Jul 7, 2015

Contributor

That's correct, clang throws a warning on cases for enums when they lack cases for all values. I can disable the warning and revert these changes if you'd like to keep this purely functional, as above with the commenting after #ifdef.

This comment has been minimized.

@CarolEidt

CarolEidt Jul 7, 2015

Member

For this kind of case, you should use unreached() - see the JIT coding conventions here: https://github.com/dotnet/coreclr/blob/master/Documentation/coding-guidelines/clr-jit-coding-conventions.md

@CarolEidt

CarolEidt Jul 7, 2015

Member

For this kind of case, you should use unreached() - see the JIT coding conventions here: https://github.com/dotnet/coreclr/blob/master/Documentation/coding-guidelines/clr-jit-coding-conventions.md

Show outdated Hide outdated src/jit/codegenlegacy.cpp
@@ -1395,7 +1397,13 @@ bool CodeGen::genMakeIndAddrMode(GenTreePtr addr,
unsigned mul;
GenTreePtr tmp;
int ixv = INT_MAX; // unset value
int ixv = INT_MAX; // unset value

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Whitespace.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Whitespace.

@@ -15952,7 +15968,7 @@ void CodeGen::genEmitHelperCall(unsigned helper,
void * addr = NULL, **pAddr = NULL;
// Don't ask VM if it hasn't requested ELT hooks
#if defined(_TARGET_ARM_) && defined(DEBUG)
#if defined(_TARGET_ARM_) && defined(DEBUG) && defined(PROFILING_SUPPORTED)

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Hrmm, tag microsofties to check, but this might be good to explain.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Hrmm, tag microsofties to check, but this might be good to explain.

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

The compProfilerMethHnd member is only defined if PROFILING_SUPPORTED is defined.

https://github.com/dotnet/coreclr/blob/master/src/jit/compiler.h#L7874

@benpye

benpye Jul 7, 2015

Contributor

The compProfilerMethHnd member is only defined if PROFILING_SUPPORTED is defined.

https://github.com/dotnet/coreclr/blob/master/src/jit/compiler.h#L7874

Show outdated Hide outdated src/jit/compiler.h
#else
var_types GetHfaType(CORINFO_CLASS_HANDLE hClass);
unsigned GetHfaSlots(CORINFO_CLASS_HANDLE hClass);
#endif

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

We may want to fix the declaration location of these depending on how perf sensitive they are.

@kangaroo

kangaroo Jul 7, 2015

Contributor

We may want to fix the declaration location of these depending on how perf sensitive they are.

This comment has been minimized.

@jkotas

jkotas Jul 7, 2015

Member

inline does nothing for Visual C++ here. Could you please make this change unconditionally for all compilers?

@jkotas

jkotas Jul 7, 2015

Member

inline does nothing for Visual C++ here. Could you please make this change unconditionally for all compilers?

Show outdated Hide outdated src/jit/emit.h
@@ -932,7 +932,7 @@ class emitter
}
void iiaSetInstrCount(int count)
{
assert(abs(count < 10));
assert(count < 10);

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

This comment has been minimized.

@CarolEidt

CarolEidt Jul 7, 2015

Member

I presume that this change is motivated by another compiler/tool error. I am not totally familiar with this code, but my assumption is that it should be assert((count >=0 && (count < 10)). A quick scan didn't indicate any context in which it should be negative.

@CarolEidt

CarolEidt Jul 7, 2015

Member

I presume that this change is motivated by another compiler/tool error. I am not totally familiar with this code, but my assumption is that it should be assert((count >=0 && (count < 10)). A quick scan didn't indicate any context in which it should be negative.

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

Yeah clang was throwing a warning here too, I can however reflect @CarolEidt s suggestion if that is correct, there were a couple of cases like this.

@benpye

benpye Jul 7, 2015

Contributor

Yeah clang was throwing a warning here too, I can however reflect @CarolEidt s suggestion if that is correct, there were a couple of cases like this.

This comment has been minimized.

@benpye

benpye Jul 13, 2015

Contributor

Further investigation shows that this should be assert(abs(count) < 10). A negative value for count can occur, at least comments indicate such, it is called from https://github.com/dotnet/coreclr/blob/master/src/jit/emitarm.cpp#L4283 where as per the comment, a negative count indicates it should jump back count instructions.

@benpye

benpye Jul 13, 2015

Contributor

Further investigation shows that this should be assert(abs(count) < 10). A negative value for count can occur, at least comments indicate such, it is called from https://github.com/dotnet/coreclr/blob/master/src/jit/emitarm.cpp#L4283 where as per the comment, a negative count indicates it should jump back count instructions.

Show outdated Hide outdated src/jit/regalloc.cpp
VarSetOps::MakeSingleton(this, tgtIndex1);
noway_assert(tgtVar->lvVarIndex == tgtIndex1);
noway_assert(tgtVar->lvRegNum != REG_STK); /* Must have been enregistered */

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Whitespace.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Whitespace.

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

This is not just whitespace, I added a scope (see the additional {}), so that the declaration isn't in the same scope as the label for the goto, can hoist the definitions above the goto if that's preferable.

@benpye

benpye Jul 7, 2015

Contributor

This is not just whitespace, I added a scope (see the additional {}), so that the declaration isn't in the same scope as the label for the goto, can hoist the definitions above the goto if that's preferable.

This comment has been minimized.

@ghost

ghost Jul 7, 2015

line 1937 seems to have an extra space.

@ghost

ghost Jul 7, 2015

line 1937 seems to have an extra space.

noway_assert((type != TYP_LONG) || (tgtVar->TypeGet() == TYP_LONG));
// On amd64 we have the occasional spec-allowed implicit conversion from TYP_I_IMPL to TYP_INT
// so this assert is meaningless
noway_assert((type != TYP_LONG) || (tgtVar->TypeGet() == TYP_LONG));

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Whitespace.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Whitespace.

/* We will rely on the target enregistered variable from the GT_ASG */
varDsc = tgtVar;
}

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Lots of whitespace :)

@kangaroo

kangaroo Jul 7, 2015

Contributor

Lots of whitespace :)

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

This and the previous are both due to adding the additional scope.

@benpye

benpye Jul 7, 2015

Contributor

This and the previous are both due to adding the additional scope.

@@ -5641,7 +5646,7 @@ regMaskTP Compiler::rpPredictAssignRegVars(regMaskTP regAvail)
// otherwise we will spill this callee saved registers,
// because its uses when combined with the uses of
// other yet to be processed candidates exceed our threshold.
totalRefCntWtd = totalRefCntWtd;
// totalRefCntWtd = totalRefCntWtd;

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

This comment has been minimized.

@CarolEidt

CarolEidt Jul 7, 2015

Member

@briansull - do you know why this assignment was there in the first place? I'm guessing that this is being deleted due to a compiler or tool error.

@CarolEidt

CarolEidt Jul 7, 2015

Member

@briansull - do you know why this assignment was there in the first place? I'm guessing that this is being deleted due to a compiler or tool error.

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

Compiler warning, can be suppressed but I would of thought a compiler would optimize it out anyway?

@benpye

benpye Jul 7, 2015

Contributor

Compiler warning, can be suppressed but I would of thought a compiler would optimize it out anyway?

Show outdated Hide outdated src/jit/target.h
// TODO: This is a hack, what's the correct solution?
#ifdef __int64
#undef __int64
#endif

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

@kangaroo

kangaroo Jul 7, 2015

Contributor

Why?

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

We define __int64 to unsigned long long, this breaks the __int64(x) calls in registerarm.h . Not sure what is the correct solution here.

@benpye

benpye Jul 7, 2015

Contributor

We define __int64 to unsigned long long, this breaks the __int64(x) calls in registerarm.h . Not sure what is the correct solution here.

This comment has been minimized.

@jkotas

jkotas Jul 7, 2015

Member

I would change the offending casts to (__int64)x

@jkotas

jkotas Jul 7, 2015

Member

I would change the offending casts to (__int64)x

@@ -71,7 +71,8 @@ class UnwindBase
UnwindBase() { }
~UnwindBase() { }
#ifdef DEBUG
// TODO: How do we get the ability to access uwiComp without error on Clang?
#if defined(DEBUG) && !defined(__GNUC__)

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Please fix the TODO.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Please fix the TODO.

This comment has been minimized.

@benpye

benpye Jul 7, 2015

Contributor

I have currently just disabled, any suggestion for how it would be possible to get uwiComp as it appears to be a circular include. Or am I better just disabling in this case as it appears to be debug specific anyway.

@benpye

benpye Jul 7, 2015

Contributor

I have currently just disabled, any suggestion for how it would be possible to get uwiComp as it appears to be a circular include. Or am I better just disabling in this case as it appears to be debug specific anyway.

@@ -3401,7 +3587,6 @@ VirtualProtect(
IN DWORD flNewProtect,
OUT PDWORD lpflOldProtect);
#if defined(_AMD64_)

This comment has been minimized.

@kangaroo

kangaroo Jul 7, 2015

Contributor

Is this true for all platforms, or just AMD64 and ARM?

@kangaroo

kangaroo Jul 7, 2015

Contributor

Is this true for all platforms, or just AMD64 and ARM?

This comment has been minimized.

@jkotas

jkotas Jul 7, 2015

Member

The GC is using it on all platforms.

@jkotas

jkotas Jul 7, 2015

Member

The GC is using it on all platforms.

@benpye

This comment has been minimized.

Show comment
Hide comment
@benpye

benpye Jul 14, 2015

Contributor

Alright, my only concern with doing such things is that later I can hit something, and be confused not knowing where it's coming from because I have done something like that, but I guess I'll have to see.

Contributor

benpye commented Jul 14, 2015

Alright, my only concern with doing such things is that later I can hit something, and be confused not knowing where it's coming from because I have done something like that, but I guess I'll have to see.

@kangaroo

This comment has been minimized.

Show comment
Hide comment
@kangaroo

kangaroo Jul 15, 2015

Contributor

@benpye I've added some additional feature specific comments. Also, I think RtlVirtualUnwind should go into the initial PR (especially considering you've got it done).

Thoughts?

Contributor

kangaroo commented Jul 15, 2015

@benpye I've added some additional feature specific comments. Also, I think RtlVirtualUnwind should go into the initial PR (especially considering you've got it done).

Thoughts?

@benpye

This comment has been minimized.

Show comment
Hide comment
@benpye

benpye Jul 15, 2015

Contributor

I'm happy to add it, only reason I didn't was that I wanted there to be some end point to this PR instead of continuous changes to review. If it's better I have that commit here I will.

Contributor

benpye commented Jul 15, 2015

I'm happy to add it, only reason I didn't was that I wanted there to be some end point to this PR instead of continuous changes to review. If it's better I have that commit here I will.

@benpye benpye referenced this pull request Jul 15, 2015

Closed

CoreFX 32 bit support #2362

@benpye

This comment has been minimized.

Show comment
Hide comment
@benpye

benpye Jul 15, 2015

Contributor

Submitted some related CoreFX changes at dotnet/corefx#2362

Contributor

benpye commented Jul 15, 2015

Submitted some related CoreFX changes at dotnet/corefx#2362

@sergiy-k

This comment has been minimized.

Show comment
Hide comment
@sergiy-k

sergiy-k Jul 19, 2015

Contributor

Impressive work!!! Very cool!
Congratulations on getting 'Hello, World!' running. 👏

Contributor

sergiy-k commented Jul 19, 2015

Impressive work!!! Very cool!
Congratulations on getting 'Hello, World!' running. 👏

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 19, 2015

Is there a checklist somewhere what has been done and what are the pending blockers? It would be really handy for the followers of this PR.

ghost commented Jul 19, 2015

Is there a checklist somewhere what has been done and what are the pending blockers? It would be really handy for the followers of this PR.

@shahid-pk

This comment has been minimized.

Show comment
Hide comment
@shahid-pk

shahid-pk Jul 19, 2015

Contributor

@jasonwilliams200OK the unwinding stuff both the jitted code unwinder and native unwinder are still to be implemented also exception handling which depends on unwinding. But both sides agree that stuff should go in the next pr.

Contributor

shahid-pk commented Jul 19, 2015

@jasonwilliams200OK the unwinding stuff both the jitted code unwinder and native unwinder are still to be implemented also exception handling which depends on unwinding. But both sides agree that stuff should go in the next pr.

@benpye

This comment has been minimized.

Show comment
Hide comment
@benpye

benpye Jul 19, 2015

Contributor

@shahid-pk is mostly right. In another branch I believe I have the unwinder for the jitted code working, and the native code unwinder should work if I can get past https://llvm.org/bugs/show_bug.cgi?id=24146 . Another thing we need to handle is what I asked @jkotas about above, there is no secure delegate stub currently which ARM uses to create a new frame to preserve some registers. CoreFX isn't very 32 bit friendly at this time, but they appear to have a plan there. Thanks @sergiy-k

Contributor

benpye commented Jul 19, 2015

@shahid-pk is mostly right. In another branch I believe I have the unwinder for the jitted code working, and the native code unwinder should work if I can get past https://llvm.org/bugs/show_bug.cgi?id=24146 . Another thing we need to handle is what I asked @jkotas about above, there is no secure delegate stub currently which ARM uses to create a new frame to preserve some registers. CoreFX isn't very 32 bit friendly at this time, but they appear to have a plan there. Thanks @sergiy-k

@kangaroo

This comment has been minimized.

Show comment
Hide comment
@kangaroo

kangaroo Jul 21, 2015

Contributor

JFYI -- I'm working on a follow on branch to this for aarch64/linux support:

https://github.com/kangaroo/coreclr/tree/aarch64

I won't PR until we resolve the issues here, since I'm building on a lot of the work @benpye has done.

Contributor

kangaroo commented Jul 21, 2015

JFYI -- I'm working on a follow on branch to this for aarch64/linux support:

https://github.com/kangaroo/coreclr/tree/aarch64

I won't PR until we resolve the issues here, since I'm building on a lot of the work @benpye has done.

@BruceForstall

This comment has been minimized.

Show comment
Hide comment
@BruceForstall

BruceForstall Jul 21, 2015

Contributor

The JIT team has one major concern with this work: it enables the LEGACY_BACKEND JIT, which we do not want to support going forward. In fact, we would like to delete all the LEGACY_BACKEND code at some point, and sooner rather than later. It still exists only for historical reasons (it provided some benefits for bringing up the RyuJIT front-end before the full RyuJIT AMD64 back-end was built).

The real path forward is to create a full RyuJIT ARM back-end using the new LSRA register allocator and linear code generation architecture. Someone really needs to sign up to do that work if the ARM port is going to be an ongoing project.

(Note that the AArch64 JIT in the port @kangaroo mentions above is a full RyuJIT architecture, but also note that it is incomplete. One strategy for building an ARM (32) RyuJIT might be to start from that.)

Contributor

BruceForstall commented Jul 21, 2015

The JIT team has one major concern with this work: it enables the LEGACY_BACKEND JIT, which we do not want to support going forward. In fact, we would like to delete all the LEGACY_BACKEND code at some point, and sooner rather than later. It still exists only for historical reasons (it provided some benefits for bringing up the RyuJIT front-end before the full RyuJIT AMD64 back-end was built).

The real path forward is to create a full RyuJIT ARM back-end using the new LSRA register allocator and linear code generation architecture. Someone really needs to sign up to do that work if the ARM port is going to be an ongoing project.

(Note that the AArch64 JIT in the port @kangaroo mentions above is a full RyuJIT architecture, but also note that it is incomplete. One strategy for building an ARM (32) RyuJIT might be to start from that.)

@benpye

This comment has been minimized.

Show comment
Hide comment
@benpye

benpye Jul 21, 2015

Contributor

I see how that might be an issue. Do you know the status of the current ARM RyuJIT back-end further than incomplete? I did at one point attempt to use the non legacy JIT, but the ARM lowering was almost all not implemented, I didn't look any further but I'm guessing that isn't the only work left to be done? It's unfortunate that the history on the JIT files doesn't go back before the initial open sourcing as it makes it somewhat difficult to understand the changes, in doing this so far I've found being able to refer to the progress on the AMD64 side has been helpful.

Other than finishing the ARM RyuJIT port, would it be possible to open jit32, or is that something you wouldn't want to support either?

Contributor

benpye commented Jul 21, 2015

I see how that might be an issue. Do you know the status of the current ARM RyuJIT back-end further than incomplete? I did at one point attempt to use the non legacy JIT, but the ARM lowering was almost all not implemented, I didn't look any further but I'm guessing that isn't the only work left to be done? It's unfortunate that the history on the JIT files doesn't go back before the initial open sourcing as it makes it somewhat difficult to understand the changes, in doing this so far I've found being able to refer to the progress on the AMD64 side has been helpful.

Other than finishing the ARM RyuJIT port, would it be possible to open jit32, or is that something you wouldn't want to support either?

@BruceForstall

This comment has been minimized.

Show comment
Hide comment
@BruceForstall

BruceForstall Jul 21, 2015

Contributor

The ARM RyuJIT port was started at one point, but never got much farther than getting a simple "hello world" program to work. The ARM64 port is much further along than that.

There have been various discussions about open sourcing JIT32, but we really don't want to do that because, once again, that's a legacy codebase. We really want all work and all innovation going into RyuJIT going forward.

Contributor

BruceForstall commented Jul 21, 2015

The ARM RyuJIT port was started at one point, but never got much farther than getting a simple "hello world" program to work. The ARM64 port is much further along than that.

There have been various discussions about open sourcing JIT32, but we really don't want to do that because, once again, that's a legacy codebase. We really want all work and all innovation going into RyuJIT going forward.

@benpye

This comment has been minimized.

Show comment
Hide comment
@benpye

benpye Jul 21, 2015

Contributor

Would it be viable to merge this if I disabled the use of LEGACY_BACKEND? It would result in this being mostly non functional (hitting an NYI in lowering IIRC), but it wouldn't use the legacy code, would stop the build being broken at the very least, and could enable @kangaroo to get somewhere getting his changes merged. I (and potentially others) would then be able to submit further PRs to get the JIT to a working state.

Contributor

benpye commented Jul 21, 2015

Would it be viable to merge this if I disabled the use of LEGACY_BACKEND? It would result in this being mostly non functional (hitting an NYI in lowering IIRC), but it wouldn't use the legacy code, would stop the build being broken at the very least, and could enable @kangaroo to get somewhere getting his changes merged. I (and potentially others) would then be able to submit further PRs to get the JIT to a working state.

@BruceForstall

This comment has been minimized.

Show comment
Hide comment
@BruceForstall

BruceForstall Jul 21, 2015

Contributor

What you suggest is very reasonable.

Certainly, the ARM LEGACY_BACKEND code exists, and is very functional and complete. There's no reason you can't use it to make progress on other parts of the port. I just want to be clear that long-term, we want it to go away in favor of a full RyuJIT ARM port.

One option is to go ahead and merge with LEGACY_BACKEND enabled. I wouldn't want us to commit to keeping it building and working, though, so I wouldn't want to see a CI build and test with this defined. Another option is to merge with this not enabled, but people working on an ARM port could enable it locally to make non-JIT progress while a full RyuJIT port is also worked on.

Contributor

BruceForstall commented Jul 21, 2015

What you suggest is very reasonable.

Certainly, the ARM LEGACY_BACKEND code exists, and is very functional and complete. There's no reason you can't use it to make progress on other parts of the port. I just want to be clear that long-term, we want it to go away in favor of a full RyuJIT ARM port.

One option is to go ahead and merge with LEGACY_BACKEND enabled. I wouldn't want us to commit to keeping it building and working, though, so I wouldn't want to see a CI build and test with this defined. Another option is to merge with this not enabled, but people working on an ARM port could enable it locally to make non-JIT progress while a full RyuJIT port is also worked on.

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Jul 22, 2015

Member

@benpye I think that this PR is pretty close. I would suggest that it is prepared for merge: Edit the title and PR description, resolve conflicts, and cleanup the history - maybe squashing it into single commit would be easiest.

I am not sure whether there are still any contentious changes that needs to be fixed up - if there are any, I would recommend to exclude them from this PR and move them into the next one.

Member

jkotas commented Jul 22, 2015

@benpye I think that this PR is pretty close. I would suggest that it is prepared for merge: Edit the title and PR description, resolve conflicts, and cleanup the history - maybe squashing it into single commit would be easiest.

I am not sure whether there are still any contentious changes that needs to be fixed up - if there are any, I would recommend to exclude them from this PR and move them into the next one.

@kangaroo

This comment has been minimized.

Show comment
Hide comment
@kangaroo

kangaroo Jul 22, 2015

Contributor

I agree with @jkotas, I think after a cleanup and squash we can do one more pass for minor things.

Contributor

kangaroo commented Jul 22, 2015

I agree with @jkotas, I think after a cleanup and squash we can do one more pass for minor things.

@benpye benpye changed the title from [WIP] CoreCLR building on ARM to CoreCLR building on ARM Jul 22, 2015

@benpye

This comment has been minimized.

Show comment
Hide comment
@benpye

benpye Jul 22, 2015

Contributor

Squashed down to three commits, split out the va_list changes as that was a violation of the C specification, and the test fix as that was an error regardless. @jkotas @kangaroo

Contributor

benpye commented Jul 22, 2015

Squashed down to three commits, split out the va_list changes as that was a violation of the C specification, and the test fix as that was an error regardless. @jkotas @kangaroo

@benpye

This comment has been minimized.

Show comment
Hide comment
@benpye

benpye Jul 24, 2015

Contributor

@janvorli I believe I have fixed the few issues you picked up upon, is there anything else or shall I squash down again?

Contributor

benpye commented Jul 24, 2015

@janvorli I believe I have fixed the few issues you picked up upon, is there anything else or shall I squash down again?

@janvorli

This comment has been minimized.

Show comment
Hide comment
@janvorli

janvorli Jul 24, 2015

Member

@benpye I am fine with it. Thanks a lot again for all the work!

Member

janvorli commented Jul 24, 2015

@benpye I am fine with it. Thanks a lot again for all the work!

@benpye

This comment has been minimized.

Show comment
Hide comment
@benpye

benpye Jul 24, 2015

Contributor

Squashed again down to the 3 commits.

Contributor

benpye commented Jul 24, 2015

Squashed again down to the 3 commits.

benpye added some commits Jul 1, 2015

Add ARM target for CoreCLR on Linux.
c_runtime/vprintf/test1 is disabled as casting NULL to va_list is
against the C specification.

Fix SetFilePointer tests on 32 bit platforms.

Define _FILE_OFFSET_BITS=64 so that we have long file support on 32 bit
platforms.

Implement context capture/restore for ARM.

Link libgcc_s before libunwind on ARM so C++ exceptions work.

Translate armasm to gas syntax.

Specify Thumb, VFPv3, ARMv7 for the ARM target.

Add ARM configuration to mscorlib build

Implement GetLogicalProcessorCacheSizeFromOS in PAL.

Set UNWIND_CONTEXT_IS_UCONTEXT_T from configure check.
After calls to vprintf (and similar) call va_end
Add the arg remover so that the arguments handled by the native vprintf
functions are removed from the arg list.
Fix FILECanonicalizePath test.
The string buffer in the test did not allow for the null terminating
byte.
@benpye

This comment has been minimized.

Show comment
Hide comment
@benpye

benpye Jul 24, 2015

Contributor

And merge conflict fixed, System.Security.Cryptography.Native was moved to corefx but the changes I made seem to not be applicable there anyway as they have removed the -DBIT64 definition.

Contributor

benpye commented Jul 24, 2015

And merge conflict fixed, System.Security.Cryptography.Native was moved to corefx but the changes I made seem to not be applicable there anyway as they have removed the -DBIT64 definition.

jkotas added a commit that referenced this pull request Jul 24, 2015

@jkotas jkotas merged commit 2cf141d into dotnet:master Jul 24, 2015

1 check passed

default Build finished. No test results found.
Details
@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Jul 24, 2015

Member

Thank you for getting this done and incorporating all feedback!

Member

jkotas commented Jul 24, 2015

Thank you for getting this done and incorporating all feedback!

@OtherCrashOverride

This comment has been minimized.

Show comment
Hide comment
@OtherCrashOverride

OtherCrashOverride Jul 24, 2015

Now that its merged, can someone provide a synopsis of what is accomplished? Does this make AArch32 JIT and execute HelloWorld?

OtherCrashOverride commented Jul 24, 2015

Now that its merged, can someone provide a synopsis of what is accomplished? Does this make AArch32 JIT and execute HelloWorld?

@benpye

This comment has been minimized.

Show comment
Hide comment
@benpye

benpye Jul 24, 2015

Contributor

@OtherCrashOverride See my last comment on #1192 , I think it covers what you are asking.

Contributor

benpye commented Jul 24, 2015

@OtherCrashOverride See my last comment on #1192 , I think it covers what you are asking.

@OtherCrashOverride

This comment has been minimized.

Show comment
Hide comment