Replace some sizeof-s with target specific constants #18245

echesakov · 2018-06-01T22:34:34Z

The goal of this PR is to identify all places in VM using sizeof operator that should be made target-specific in order to support cross-bitness scenario #16513. ~~The proposal is to introduce CrossBitness_SizeOf operator to make such places explicit.~~

~~CrossBitness_SizeOf operator satisfies the following conditions:~~

~~CrossBitness_SizeOf(X) == sizeof(X) in non-crossbitness scenarios;~~

CrossBitness_SizeOf(X) == sizeof(Y) where Y is a "fake" structure created to describe layout of X on the target platform in crossbitness scenarios (i.e. x64_arm). Note that usually sizeof(X) != sizeof(Y).

It's been proven experimentally that this subset of changes is needed in order to get outputs of x64_arm and arm_arm crossgens to be binary identical for S.P.C.dll and CoreFx assemblies.

This PR also includes other related changes:

LOG2_PTRSIZE should depend on _TARGET_64BIT_;
more careful pointer arithmetic in FakePromote and ReportPointersFromValueType.

~~This PR is to initiate design discussion (at alternatives discussion) and not a final work.~~

Update: the final solution is to replace identified sizeof expressions with target-specifics constants

@jkotas PTAL

…mpile.cpp

jkotas · 2018-06-01T23:15:45Z

src/vm/ceeload.cpp

@@ -2139,7 +2139,7 @@ void Module::BuildStaticsOffsets(AllocMemTracker *pamTracker)
                        (S_SIZE_T(2 * sizeof(DWORD))*(S_SIZE_T(dwNumTypes)+S_SIZE_T(1)))));

                for (DWORD i = 0; i < dwIndex; i++) {
-                    pRegularStaticOffsets[i * 2    ] = dwGCHandles[0]*sizeof(OBJECTREF);


sizeof(OBJECTREF) can be just TARGET_POINTER_SIZE. We use TARGET_POINTER_SIZE for object reference sizes in many places already, so a few more are not a concern.

Sure, I will replace with TARGET_POINTER_SIZE instead

jkotas · 2018-06-01T23:17:24Z

src/vm/crossbitness.h

+#endif // _TARGET_64BIT_
+    };
+
+    struct StringObject : public Object


I do not see this used.

I guess it was needed before #17876

Yes, you're right. It was needed before for ObjSizeOf(StringObject) in StringObject::GetSize.

jkotas · 2018-06-01T23:22:43Z

src/vm/crossbitness.h

+#ifndef CROSSBITNESS_H
+#define CROSSBITNESS_H
+
+struct CrossBitness


I would rather use explicitly manually defined sizes than trust the C++ compiler that it is going to layout the structures the way we expect.

Building upon approach I took in https://github.com/dotnet/coreclr/pull/17876/files .

@jkotas I also though about this alternative. But then I would also need to replace all ObjSizeOf(Object) and ObjSizeOf(ArrayBase) expressions with some constants as well.

Is any standard name for sizeof(Object) + sizeof(ObjHeader)?

jkotas · 2018-06-02T01:19:09Z

sizeof(Object) + sizeof(ObjHeader)

I would call it OBJECT_BASE_SIZE.

jkotas · 2018-06-02T01:21:12Z

Or Object::GetBaseSize()

jkotas · 2018-06-02T01:23:50Z

BTW: How far do you think you are right now - are there things that you have working locally?

echesakov · 2018-06-02T01:35:57Z

@jkotas The x64_arm crossgen version in https://github.com/echesakovMSFT/coreclr/tree/CrossCrossGen compiles and runs (at least on Windows) and I can crossgen S.P.C.dll and all CoreFx assemblies

echesakov · 2018-06-02T02:09:55Z

@jkotas Regarding placements such constants - would asmconstants.h be appropriate file? Or I can keep them in crossbitness.h (for 32 and 64 bit targets) ?

jkotas · 2018-06-02T03:38:19Z

asmconstants.h is per architecture, so it would need to be duplicated.

I would probably just add them at the top of object.h before MIN_OBJECT_SIZE, something like:

#define OBJHEADER_SIZE      TARGET_POINTER_SIZE /* ObjHeader */

#define OBJECT_SIZE         TARGET_POINTER_SIZE /* MethodTable* */
#define OBJECT_BASESIZE     (OBJHEADER_SIZE + OBJECT_SIZE)

#define ARRAYBASE_SIZE      (OBJECT_SIZE /* PTR_MethodTable */ + TARGET_POINTER_SIZE /* _numComponents + pad */)
#define ARRAYBASE_BASESIZE  (OBJHEADER_SIZE + ARRAYBASE_SIZE)

#define MIN_OBJECT_SIZE     (2*TARGET_POINTER_SIZE + OBJHEADER_SIZE)

jkotas · 2018-06-02T03:39:31Z

src/vm/object.inl

@@ -257,7 +257,7 @@ inline /* static */ unsigned ArrayBase::GetBoundsOffset(MethodTable* pMT)
    if (!pMT->IsMultiDimArray()) 
        return(offsetof(ArrayBase, m_NumComponents));
    _ASSERTE(pMT->GetInternalCorElementType() == ELEMENT_TYPE_ARRAY);


You can also fix offsetof(ArrayBase, m_NumComponents) a few lines above while you are on it. Replace it with OBJECT_SIZE /* offset(ArrayBase, m_NumComponents */ ?

Done. Thanks for catching this

echesakov · 2018-06-02T20:54:43Z

After I replaced ObjSizeOf and CrossBitness_SizeOf with predefined constants I think next logical step should be adding some kind of checking (either at compile time or running time) that the values of these constants stay valid. For example,

C_ASSERT(sizeof(ObjHeader) == OBJHEADER_SIZE);
C_ASSERT(sizeof(Object) == OBJECT_SIZE);

However, such assertions would be valid only for non-crossbitness scenarios (so I can limit them to such scenarois only with ifdefs).

@jkotas What do you think?

jkotas · 2018-06-02T21:40:00Z

You can add these static asserts somewhere if you would like.

I am not really concerned about these particular constants. This is unlikely to change; and if it does change and the constants get out of sync by accident, it will be easy to notice because of nothing will work.

* OBJHEADER_SIZE * OBJECT_SIZE * OBJECT_BASESIZE * ARRAYBASE_SIZE * ARRAYBASE_BASESIZE and use them instead of corresponding expressions * sizeof(ObjHeader) * sizeof(Object) * ObjSizeOf(Object) * sizeof(ArrayBase) * ObjSizeOf(ArrayBase)

echesakov · 2018-06-03T01:14:36Z

@jkotas Agree, this assertions would not be that helpful. I rebased and squashed some commits and wait for green ci. Thanks for the help!

* Replace sizeof expressions with target-specific constants: * sizeof(ObjHeader) -> OBJHEADER_SIZE * sizeof(Object) -> OBJECT_SIZE * ObjSizeOf(Object) -> OBJECT_BASESIZE * sizeof(ArrayBase) -> ARRAYBASE_SIZE * ObjSizeOf(ArrayBase) -> ARRAYBASE_BASESIZE * Remove ObjSizeOf macro * Use OBJECT_SIZE in ArrayBase::GetBoundsOffset * Cast ppObj to CORCOMPILE_GCREFMAP_TOKENS* before dereferencing in FakePromote * LOG2_PTRSIZE should depend on _TARGET_64BIT_ Commit migrated from dotnet/coreclr@de58676

Cast ppObj to CORCOMPILE_GCREFMAP_TOKENS* in FakePromote in src/vm/co…

0ce6199

…mpile.cpp

echesakov added area-VM Design Discussion Ongoing discussion about design without consensus labels Jun 1, 2018

jkotas reviewed Jun 1, 2018

View reviewed changes

jkotas reviewed Jun 2, 2018

View reviewed changes

jkotas approved these changes Jun 2, 2018

View reviewed changes

echesakov added 4 commits June 2, 2018 18:04

LOG2_PTRSIZE depends on _TARGET_64BIT_

670290b

Define target-specific constants

239c165

* OBJHEADER_SIZE * OBJECT_SIZE * OBJECT_BASESIZE * ARRAYBASE_SIZE * ARRAYBASE_BASESIZE and use them instead of corresponding expressions * sizeof(ObjHeader) * sizeof(Object) * ObjSizeOf(Object) * sizeof(ArrayBase) * ObjSizeOf(ArrayBase)

Remove ObjSizeOf

39d3920

Use OBJECT_SIZE in ArrayBase::GetBoundsOffset

f49e714

echesakov changed the title ~~[WIP] Introduce CrossBitness_SizeOf operator to support CrossBitness CrossGen~~ Replace some sizeof-s with target specific constants Jun 3, 2018

echesakov removed the Design Discussion Ongoing discussion about design without consensus label Jun 3, 2018

echesakov merged commit de58676 into dotnet:master Jun 4, 2018

echesakov deleted the CrossBitnessSizeOf branch June 4, 2018 17:08

echesakov mentioned this pull request Jun 8, 2018

Cross-bitness in instance fields placement and CORINFO structs #18366

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace some sizeof-s with target specific constants #18245

Replace some sizeof-s with target specific constants #18245

echesakov commented Jun 1, 2018 •

edited

jkotas Jun 1, 2018

echesakov Jun 2, 2018

jkotas Jun 1, 2018

jkotas Jun 1, 2018

echesakov Jun 2, 2018 •

edited

jkotas Jun 1, 2018

echesakov Jun 2, 2018 •

edited

jkotas commented Jun 2, 2018

jkotas commented Jun 2, 2018

jkotas commented Jun 2, 2018

echesakov commented Jun 2, 2018

echesakov commented Jun 2, 2018

jkotas commented Jun 2, 2018

jkotas Jun 2, 2018

echesakov Jun 2, 2018

echesakov commented Jun 2, 2018

jkotas commented Jun 2, 2018 •

edited

echesakov commented Jun 3, 2018 •

edited

Replace some sizeof-s with target specific constants #18245

Replace some sizeof-s with target specific constants #18245

Conversation

echesakov commented Jun 1, 2018 • edited

jkotas Jun 1, 2018

Choose a reason for hiding this comment

echesakov Jun 2, 2018

Choose a reason for hiding this comment

jkotas Jun 1, 2018

Choose a reason for hiding this comment

jkotas Jun 1, 2018

Choose a reason for hiding this comment

echesakov Jun 2, 2018 • edited

Choose a reason for hiding this comment

jkotas Jun 1, 2018

Choose a reason for hiding this comment

echesakov Jun 2, 2018 • edited

Choose a reason for hiding this comment

jkotas commented Jun 2, 2018

jkotas commented Jun 2, 2018

jkotas commented Jun 2, 2018

echesakov commented Jun 2, 2018

echesakov commented Jun 2, 2018

jkotas commented Jun 2, 2018

jkotas Jun 2, 2018

Choose a reason for hiding this comment

echesakov Jun 2, 2018

Choose a reason for hiding this comment

echesakov commented Jun 2, 2018

jkotas commented Jun 2, 2018 • edited

echesakov commented Jun 3, 2018 • edited

echesakov commented Jun 1, 2018 •

edited

echesakov Jun 2, 2018 •

edited

echesakov Jun 2, 2018 •

edited

jkotas commented Jun 2, 2018 •

edited

echesakov commented Jun 3, 2018 •

edited