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

[CppCodeGen] Enable reflection support #6700

Merged
merged 8 commits into from Jan 10, 2019

Conversation

Projects
None yet
3 participants
@kbaladurin
Copy link
Collaborator

kbaladurin commented Dec 19, 2018

With this change and #6695 reflection tests are passed with following constraints:

  • Some tests are skipped due to missing exceptions support
  • TestByRefLikeTypeMethod is skipped (it's necessary to implement RhGetCodeTarget for CppCodeGen)
  • TestByRefReturnInvoke is skipped (it's necessary to implement Nullable box/unbox)

Also #6405 and #6423 should be fixed.

@kbaladurin kbaladurin force-pushed the kbaladurin:cppgen branch 2 times, most recently from 5c454e4 to 2d53b84 Dec 20, 2018

@@ -439,6 +446,11 @@ public sealed override MethodInvoker TryGetMethodInvoker(RuntimeTypeHandle decla
// Now have the type loader build or locate a dictionary for this method
uint index = cookie >> 1;

if (!RuntimeAugments.SupportsRelativePointers)
{
index = (uint)(IntPtr.Size * index) / sizeof(uint);

This comment has been minimized.

@MichalStrehovsky

MichalStrehovsky Dec 20, 2018

Member

Indexing into pBlob is becoming kind of awkward after this change. I think we should just change the type of pBlob to void* and cast to the right type before each use.

This comment has been minimized.

@kbaladurin

kbaladurin Dec 24, 2018

Collaborator

Done. Thank you!

}
else
{
// .NET Native uses RVAs
dynamicInvokeMethod = TypeLoaderEnvironment.RvaToFunctionPointer(module.Handle, pBlob[index + 1]);
dynamicInvokeMethod = TypeLoaderEnvironment.RvaToFunctionPointer(module.Handle, pBlob[index + IntPtr.Size / sizeof(uint)]);

This comment has been minimized.

@MichalStrehovsky

MichalStrehovsky Dec 20, 2018

Member

Why was this needed?

This comment has been minimized.

@kbaladurin

kbaladurin Dec 24, 2018

Collaborator

It's unnecessary

@kbaladurin kbaladurin force-pushed the kbaladurin:cppgen branch from 2d53b84 to 688ec83 Dec 24, 2018

@@ -279,7 +279,8 @@ private string GetStackValueKindCPPTypeName(StackValueKind kind, TypeDesc type =
case StackValueKind.Int64: return "int64_t";
case StackValueKind.NativeInt: return "intptr_t";
case StackValueKind.ObjRef: return "void*";
case StackValueKind.Float: return "double";
case StackValueKind.Float:

This comment has been minimized.

@jkotas

jkotas Dec 24, 2018

Member

Why is this needed? It would be nice to add a comment.

This comment has been minimized.

@kbaladurin

kbaladurin Dec 25, 2018

Collaborator

The problem occurs when we use reflection to get float fields. Compiler generates following code to unbox float value:

double unboxed = *(double*)(float*)((void**)boxed + 1)

that produce incorrect result.

This comment has been minimized.

@jkotas

jkotas Dec 26, 2018

Member

This should be rather fixed in the implementation of unbox instead of here.

I think that the same problem will happen for other types too. For example, if the type is sbyte and the boxed value is -1, we will end up with 0xFF pushed on the IL stack today. Instead, we should end up with -1 on the IL stack.

This comment has been minimized.

@kbaladurin

kbaladurin Dec 27, 2018

Collaborator

Thank you for suggestion!

return typeName.Replace("::", "_") + "_" + name;
string res = typeName.Replace("::", "_") + "_" + name;

if (isGCStatic && !isThreadStatic && dataNameNeeded)

This comment has been minimized.

@jkotas

jkotas Dec 24, 2018

Member

What is the __data suffix for? I am not able to figure it out by just looking at the code.

This comment has been minimized.

@kbaladurin

kbaladurin Dec 25, 2018

Collaborator

__data is used for naming variables of structure type:

_System_Private_CoreLib_System_Boolean_GCStatics _System_Private_CoreLib_System_Boolean_gcStatics__data;
_System_Private_CoreLib_System_Boolean_GCStatics *_System_Private_CoreLib_System_Boolean_gcStatics__data__ptr = &_System_Private_CoreLib_System_Boolean_gcStatics__data;
_System_Private_CoreLib_System_Boolean_GCStatics **_System_Private_CoreLib_System_Boolean_gcStatics = &_System_Private_CoreLib_System_Boolean_gcStatics__data__ptr;
symbolNode.Target is GenericTypesHashtableNode ||
symbolNode.Target is TypeMetadataMapNode ||
if ((!(symbolNode.Target is EmbeddedDataContainerNode) &&
!(symbolNode.Target is StackTraceMethodMappingNode) ||
isEagerCctorTable

This comment has been minimized.

@jkotas

jkotas Dec 24, 2018

Member

isEagerCctorTable condition does not seem to be necessary now that the logic is flipped.

This comment has been minimized.

@kbaladurin

kbaladurin Dec 25, 2018

Collaborator

isEagerCctorTable is needed because !(symbolNode.Target is EmbeddedDataContainerNode) filters all ArrayOfEmbeddedPointersNode nodes.


// Method generic dictionaries get prefixed by the hash code of the owning method
if (node is MethodGenericDictionaryNode)
offset -= _compilation.TypeSystemContext.Target.PointerSize;

This comment has been minimized.

@jkotas

jkotas Dec 24, 2018

Member

MethodGenericDictionaryNode.HeaderSize ?

This comment has been minimized.

@jkotas

jkotas Dec 24, 2018

Member

Or even better - would an unconditional call to ISymbolDefinitionNode.Offset work ?

This comment has been minimized.

@kbaladurin

kbaladurin Dec 25, 2018

Collaborator

ISymbolDefinitionNode.Offset points to the part of data after header but we doesn't know header size. I think using MethodGenericDictionaryNode.HeaderSize is more suitable, but it's necessary to make it public.

Konstantin Baladurin added some commits Dec 24, 2018

@kbaladurin kbaladurin force-pushed the kbaladurin:cppgen branch from 688ec83 to 0493f67 Dec 25, 2018

Konstantin Baladurin added some commits Dec 24, 2018

Konstantin Baladurin
[CppCodeGen] Fix boxing/unboxing
As we use double for float and int32_t for sbyte and int16 we should
use exact type for boxing/unboxing otherwise we will get incorrect
results.

This patch fixes float box/unbox issues in reflection tests

@kbaladurin kbaladurin force-pushed the kbaladurin:cppgen branch from 0493f67 to 1eeb38a Dec 27, 2018

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 10, 2019

@dotnet-bot test this please

@jkotas

jkotas approved these changes Jan 10, 2019

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 10, 2019

Thanks

@jkotas jkotas merged commit bf484d3 into dotnet:master Jan 10, 2019

12 checks passed

OSX10.12 Debug and CoreCLR tests Build finished.
Details
OSX10.12 Debug and CoreFX tests Build finished.
Details
OSX10.12 Release Build finished.
Details
Ubuntu Debug and CoreCLR tests Build finished.
Details
Ubuntu Debug and CoreFX tests Build finished.
Details
Ubuntu Release Build finished.
Details
Windows_NT Debug and CoreCLR tests Build finished.
Details
Windows_NT Debug and CoreFX tests Build finished.
Details
Windows_NT Release Build finished.
Details
Windows_NT_Wasm Debug and CoreCLR tests WebAssembly Build finished.
Details
Windows_NT_Wasm Release WebAssembly Build finished.
Details
license/cla All CLA requirements met.
Details

@kbaladurin kbaladurin deleted the kbaladurin:cppgen branch Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment