Allow users to enable automatic NI bind fallback via command-line #11485
Conversation
|
@gkhanna79 @sdmaclea Please take a look. IMHO, #11324 should be addressed before 2.0 branching out if ARM/Linux also suffers from it, but I currently have no infrastructure to test whether NI image loading works or not. I currently attempt to build up testing infrastructure, but am not sure whether it is possible before 2.0 branching out. I submitted this PR first as a workaround. |
clrfeatures.cmake
Outdated
|
|
||
| if(NOT DEFINED FEATURE_NI_BIND_FALLBACK) | ||
| if(CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_ARM64) | ||
| set(FEATURE_IMPLICIT_NI_BIND 1) |
There was a problem hiding this comment.
What is meant FEATURE_IMPLICIT_NI_BIND ?
src/binder/assemblybinder.cpp
Outdated
| @@ -689,11 +689,11 @@ namespace BINDER_SPACE | |||
| sCoreLib = sCoreLibDir; | |||
| sCoreLib.Append(CoreLibName_IL_W); | |||
| BOOL fExplicitBindToNativeImage = (fBindToNativeImage == true)? TRUE:FALSE; | |||
| #if defined(FEATURE_PAL) && !defined(_TARGET_AMD64_) && !defined(_TARGET_ARM64_) | |||
| #if defined(FEATURE_PAL) && !defined(FEATURE_NI_BIND_FALLBACK) | |||
There was a problem hiding this comment.
Semantically, "NI_BIND_FALLBACK" happens when we bind as IL, instead of NI. Thus, the define should look like the following:
#if defined(FEATURE_PAL) && defined(FEATURE_NI_BIND_FALLBACK)
clrfeatures.cmake
Outdated
| @@ -25,3 +25,9 @@ endif(NOT DEFINED FEATURE_DBGIPC) | |||
| if(NOT DEFINED FEATURE_INTERPRETER) | |||
| set(FEATURE_INTERPRETER 0) | |||
| endif(NOT DEFINED FEATURE_INTERPRETER) | |||
|
|
|||
| if(NOT DEFINED FEATURE_NI_BIND_FALLBACK) | |||
| if(CLR_CMAKE_TARGET_ARCH_AMD64 OR CLR_CMAKE_TARGET_ARCH_ARM64) | |||
There was a problem hiding this comment.
This "if" check should be NOT of the existing "if" to account for my comment at https://github.com/dotnet/coreclr/pull/11485/files#r115656309.
|
@parjong Thanks for making this - if you can submit an updated PR, we should be able to get this in by EOD tomorrow. I do think it will be valuable to validate comment from @sdmaclea at #11341 (comment) and confirm Arm32 NI works (which, I believe, it should be because I would assume Tizen would be using NI image of SPC.dll, right?). |
|
@gkhanna79 Thanks you for comments. Yes, Tizen release contains S.P.CoreLib.ni.dll in order to reduce application launching time. |
clrfeatures.cmake
Outdated
| @@ -25,3 +25,9 @@ endif(NOT DEFINED FEATURE_DBGIPC) | |||
| if(NOT DEFINED FEATURE_INTERPRETER) | |||
| set(FEATURE_INTERPRETER 0) | |||
| endif(NOT DEFINED FEATURE_INTERPRETER) | |||
|
|
|||
| if(NOT DEFINED FEATURE_NI_BIND_FALLBACK) | |||
There was a problem hiding this comment.
One more suggestion - can you make this entire check under "if not Win32" as that will allow you to remove FEATURE_PAL from https://github.com/dotnet/coreclr/pull/11485/files#diff-1708de55e5220e56e33f7ddb0f465f9cR692?
There was a problem hiding this comment.
You should move the NOT WIN32 one level up so that the entire block is within it.
There was a problem hiding this comment.
As I understand, the behavior is same if cmakeargs "-DFEATURE_NI_BIND_FALLBACK=..." is absent. Is there another issue that I am not aware of?
There was a problem hiding this comment.
Just readability - nothing else :)
There was a problem hiding this comment.
How about the following form?
if(WIN32)
set(FEATURE_NI_BIND_FALLBACK 0)
endif(WIN32)
if(NOT DEFINED FEATURE_NI_BIND_FALLBACK)
if(NOT CLR_CMAKE_TARGET_ARCH_AMD64 AND NOT CLR_CMAKE_TARGET_ARCH_ARM64)
set(FEATURE_NI_BIND_FALLBACK 1)
endif()
endif(NOT DEFINED FEATURE_NI_BIND_FALLBACK)
|
@gkhanna79 Updated as suggested :) |
This commit allows users to enable #11341 via command-line option. The default behavior remains unchanged.