Node byte data emission #1648
Node byte data emission #1648
Conversation
| case RelocType.IMAGE_REL_BASED_DIR64: | ||
| size = 8; | ||
| break; | ||
| case RelocType.IMAGE_REL_BASED_REL32: |
There was a problem hiding this comment.
I do not think IMAGE_REL_BASED_REL32 relocs are going to work for CppCodeGen.
I think you should just assert here that you are only getting pointer reloc here, something like:
Debug.Assert(reloc.RelocType == (PointerSize == 8) ? RelocType.IMAGE_REL_BASED_DIR64 : RelocType.IMAGE_REL_BASED_HIGHLOW);
A-And
force-pushed
the
NodeByteEmission
branch
2 times, most recently
from
d900ba1 to
d4235db
Compare
| @@ -70,6 +70,7 @@ struct RawEEType | |||
| uint16_t m_usNumVtableSlots; | |||
| uint16_t m_usNumInterfaces; | |||
| uint32_t m_uHashCode; | |||
| void* m_pIndirectionModule; | |||
There was a problem hiding this comment.
Nit: the indentation looks messed up here - please convert the tabs to spaces
A-And
force-pushed
the
NodeByteEmission
branch
from
305087a to
0dadfea
Compare
A-And
force-pushed
the
NodeByteEmission
branch
from
b8bc632 to
d3e844c
Compare
| { | ||
| var sb = new CppGenerationBuffer(); | ||
| List<Tuple<bool, int>> nodeDataDivs = new List<Tuple<bool, int>>(); | ||
| byte[] actualData = new byte[nodeData.Data.Length]; |
There was a problem hiding this comment.
Did you look into reducing the need for a copy of the bytes here? Ideally we only ever read bytes from nodeData.Data and don't copy them into actualData
|
@MichalStrehovsky @jkotas Could you take another look at this? |
|
@dotnet-bot test Windows_NT Debug please |
| private String GetCodeForType(TypeDesc type) | ||
| { | ||
| var sb = new CppGenerationBuffer(); | ||
| List<Tuple<bool, int>> nodeDataDivs = new List<Tuple<bool, int>>(); |
There was a problem hiding this comment.
Nit: Tuple<bool,int> is kind of hard to read - you have to keep in your head what Item1 and Item2 means. I think it would look better to define a helper struct with actual field names for it, like we do everywhere else (e.g. take struct Relocation as an example).
|
LGTM modulo a few minor comments |
| if (baseType != null) | ||
| AppendVirtualSlots(sb, implType, baseType); | ||
| // virtual slots | ||
| var nodeData = (node as ObjectNode).GetData(factory, false); |
There was a problem hiding this comment.
If ObjectNode is the only node type this method can handle, make the method take ObjectNode instead of DependencyNode?
Emitting the byte data of a node instead of generating contents "by hand". Added emission of optional type fields.
A-And
force-pushed
the
NodeByteEmission
branch
from
22381e0 to
07657b2
Compare
|
Thanks! |
Emitting the byte data of a node instead of generating contents "by hand".
@nattress @MichalStrehovsky @jkotas