-
Notifications
You must be signed in to change notification settings - Fork 32
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 ldvirtftn #141
Add ldvirtftn #141
Conversation
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 cover the semantics of ldvirtftn
adequately. It has to follow the exact same behavior as callvirt
when it comes to method devirtualization.
We may have to somehow extract the method devirtualization of callvirt
into its own separate unit such that it can be reused by both callvirt
and ldvirtftn
without having to copy implementations.
do | ||
{ | ||
// try resolve function | ||
var resolvedVirtualFunction = thisObjectType.Methods | ||
.FirstOrDefault(method => method.Name == virtualFunctionName | ||
&& SignatureComparer.Default.Equals(method.Signature, virtualFunctionSignature)); | ||
|
||
// if resolved then push function pointer | ||
if (resolvedVirtualFunction != null) | ||
{ | ||
var functionPointer = methods.GetAddress(resolvedVirtualFunction); | ||
stack.Push(factory.CreateNativeInteger(functionPointer), type); | ||
return CilDispatchResult.Success(); | ||
} | ||
|
||
// else switch to BaseType and try resolve again | ||
thisObjectType = thisObjectType.BaseType?.Resolve(); | ||
} // or exit and throw CilEmulationException | ||
while (thisObjectType != null); |
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 not sufficient. Ldvirtftn follows the exact same method devirtualization process as callvirt
, with all its edge cases.
If that is the case, we need a test case for this too. |
looks like a new issue for AsmResolver |
If that is the case, it would be great if you could make a minimal reproducible test case and put it on the AsmResolver issue board. However I often found this to be the case of Echo simply not always instantiating the generic types properly everywhere. In that case, we still need a minimal unit test, but specifically using Echo instead :) In either case, that probably warrants a separate issue, independent of the |
I poked around the error a bit and fixed it (almost hehehe). I don't know if the error is in AsmResolver or Echo +_+
|
This is very likely an Echo bug. I don't see any evidence of the actual type memory layout algorithm being problematic here. This problem was partially addressed for the Echo/src/Platforms/Echo.Platforms.AsmResolver/Emulation/Dispatch/ObjectModel/CallHandlerBase.cs Lines 151 to 206 in 687c1fe
We should probably copy/extend this to work for fields as well. Regardless, this is definitely out of scope for this PR, and really should be in a separate issue. |
Also unexpected, but ((object)anyInteger).ToString() throws exception +_+