Redirect string constructor references to the Ctor method #125
Conversation
|
I'm not removing the workaround yet because there's another issue at the BCL level. I'll create a bug # for it shortly. |
|
This is a fix for issue #116. Not sure how to associate it on GitHub. |
|
You have just associated it. Also if you say "Fixes #XXX" in your commit message, github will close the issue for you once the commit is merged - see https://help.github.com/articles/closing-issues-via-commit-messages/ |
| @@ -384,7 +384,19 @@ private void ComputeDependencyNodeDependencies(List<DependencyNodeCore<NodeFacto | |||
| object target = methodCode.Relocs[i].Target; | |||
| if (target is MethodDesc) | |||
| { | |||
There was a problem hiding this comment.
We want the reloc remapping to go away as part of #68 - it would be nice to avoid adding stuff here. It would be better to do this either earlier in getCallInfo (point codePointerOrStubLookup to the actual string method) or somewhere later when we are wiring stuff together. Not sure which one is better - doing it earlier should be easier.
There was a problem hiding this comment.
Also, it may be nice to factor this out into a helper method (class IntrinsicMethods may be a good place for it) and call it from CppCodeGen as well.
Codegen for String object construction is special - the string object allocation actually happens in the constructor call. The constructor is actually a static method named Ctor that returns a string. We need to transform calls to .ctor to the magical Ctor method.
|
Sigh, this is still not the right fix. JIT generates code that puts a null this pointer into RCX, so the arguments actually get shifted and don't match what Ctor expects. Should we add a dummy this pointer to the Ctor(...) methods or do we want to fix this on the codegen side? |
Yes, at least for now. |
| @@ -1484,8 +1484,16 @@ void getCallInfo(IntPtr _this, ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINF | |||
| } | |||
| else | |||
| { | |||
| _compilation.AddMethod(method); | |||
| pResult.codePointerOrStubLookup.constLookup.addr = pResolvedToken.hMethod; | |||
| if (!method.IsConstructor || !method.OwningType.IsString) | |||
There was a problem hiding this comment.
Nit - the condition may be easier to read if it is "positive": if (method.IsConstructor && method.OwningType.IsString)
Add a 'this' pointer to the static methods that are used as String constructors to match RyuJIT expectations. RyuJIT passes a useless 'this' in RCX. This leads to an unfortunate complication in the C++ backend. I tried to keep the workaround contained in a single place. Along with the redirection checked in a separate commit, resolves #116.
Redirect string constructor references to the Ctor method
Codegen for String object construction is special - first we allocate
the string as an array and then we call a magical Ctor static method to
initialize the contents. We need to do the .ctor -> Ctor transformation at
the call sites.