Skip to content

Fix FCall implementation collision on ARM64 #39810

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.

Sign up for GitHub

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

Jump to bottom
Merged
AntonLapounov merged 1 commit into from
Jul 23, 2020
Merged

Fix FCall implementation collision on ARM64 #39810

AntonLapounov merged 1 commit into from
Jul 23, 2020

Conversation

@AntonLapounov
Copy link
Member

@AntonLapounov AntonLapounov commented Jul 23, 2020

Fixes #39701. Two FCalls, DependentHandle::nSetPrimary and MarshalNative::GCHandleInternalSet, had identical implementations, which led to a failure in ECall::GetFCallImpl ("Duplicate pImplementation entries found in reverse fcall table"). Add FCUnique() to make them different.

@AntonLapounov AntonLapounov requested a review from jkotas July 23, 2020 02:29
jkotas
jkotas approved these changes Jul 23, 2020
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@jkotas
Copy link
Member

jkotas commented Jul 23, 2020

Just curious - how was this leading to COMException?

@mangod9
Copy link
Member

mangod9 commented Jul 23, 2020

also why is this arm64 specific?

@AntonLapounov
Copy link
Member Author

AntonLapounov commented Jul 23, 2020

@jkotas ThrowHR(E_FAIL) in ECall::GetFCallImpl is translated to a COMException, which is caught by BinaryFormatter.Deserialize and wrapped with a SerializationException. I have looked through rexcep.h and could not find a more appropriate exception type. Here is the call stack when we raise the exception:

CoreCLR!ThrowHR
CoreCLR!ECall::GetFCallImpl
CoreCLR!MethodDesc::DoPrestub
CoreCLR!PreStubWorker
CoreCLR!ThePreStub
System_Private_CoreLib!System.Runtime.CompilerServices.ConditionalWeakTable`2+Container[[System.__Canon, System.Private.CoreLib],[System.__Canon, System.Private.CoreLib]].Remove(System.__Canon)
System_Private_CoreLib!System.Runtime.CompilerServices.ConditionalWeakTable`2[[System.__Canon, System.Private.CoreLib],[System.__Canon, System.Private.CoreLib]].Remove(System.__Canon)
System_Private_CoreLib!System.Collections.Hashtable.OnDeserialization(System.Object)
system_runtime_serialization_formatters!System.Runtime.Serialization.ObjectManager.RaiseDeserializationEvent()
system_runtime_serialization_formatters!System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(System.Runtime.Serialization.Formatters.Binary.BinaryParser)
system_runtime_serialization_formatters!System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(System.IO.Stream)

@mangod9 Functions must be byte-to-byte identical to be folded by the linker, so it ultimately depends on the compiler whether the two functions below produce identical code in release builds (ValidateHandleAssignment and _ASSERTE are debug-only).

FCIMPL2(VOID, DependentHandle::nSetPrimary, OBJECTHANDLE handle, Object *_primary)
{
    FCALL_CONTRACT;

    _ASSERTE(handle != NULL);

    IGCHandleManager *mgr = GCHandleUtilities::GetGCHandleManager();
    mgr->StoreObjectInHandle(handle, _primary);
}
FCIMPLEND

FCIMPL2(VOID, MarshalNative::GCHandleInternalSet, OBJECTHANDLE handle, Object *obj)
{
    FCALL_CONTRACT;

    OBJECTREF objRef(obj);
    StoreObjectInHandle(handle, objRef);
}
FCIMPLEND

inline void StoreObjectInHandle(OBJECTHANDLE handle, OBJECTREF object)
{
    ValidateHandleAssignment(handle, object);

    GCHandleUtilities::GetGCHandleManager()->StoreObjectInHandle(handle, OBJECTREFToObject(object));
}

@AntonLapounov AntonLapounov merged commit 811c8c7 into dotnet:master Jul 23, 2020
@AntonLapounov AntonLapounov deleted the Arm64_FixDuplicateFCalls branch July 23, 2020 04:17
ViktorHofer added a commit that referenced this pull request Jul 23, 2020
@ViktorHofer
Revert "Fix FCall implementation collision on ARM64 (#39810)"
ef0a45e 
This reverts commit 811c8c7.
@ViktorHofer ViktorHofer mentioned this pull request Jul 23, 2020
Closed
@jkotas
Copy link
Member

jkotas commented Jul 23, 2020

ThrowHR(E_FAIL) in ECall::GetFCallImpl is translated to a COMExceptio

Should we change ThrowHR(E_FAIL) to _ASSERTE_ALL_BUILDS or something like that to make this easier to diagnose next time?

Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
@AntonLapounov @Jacksondr5
Fix FCall implementation collision on ARM64 (dotnet#39810)
4e48225 
Two FCalls, DependentHandle::nSetPrimary and MarshalNative::GCHandleInternalSet, had identical implementations, which led to a failure in ECall::GetFCallImpl ("Duplicate pImplementation entries found in reverse fcall table"). Add FCUnique() to make them different.
@AntonLapounov AntonLapounov mentioned this pull request Aug 12, 2020
Closed
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotas jkotas jkotas approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

FCall implementation collision on Windows ARM64

4 participants

@AntonLapounov @jkotas @mangod9 @karelz