Fix Linux x86 build #50836

gbalykov · 2021-04-07T09:37:33Z

After this change next subsets can be built: clr.runtime+clr.corelib+clr.iltools+clr.dactools+clr.jit

Related issue: #40003

cc @alpencolt

ghost · 2021-04-07T09:37:39Z

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

After this change next subsets can be built: clr.runtime+clr.corelib+clr.iltools+clr.dactools+clr.jit

Related issue: #40003

cc @alpencolt

Author:	gbalykov
Assignees:	-
Labels:	`area-Infrastructure-coreclr`
Milestone:	-

src/coreclr/vm/jithelpers.cpp

Changes have been made.

hoyosjs · 2021-04-07T21:27:40Z

src/coreclr/md/inc/metamodel.h

@@ -594,6 +594,7 @@ class CMiniMdBase : public IMetaModelCommonRO


 protected:
+    DAC_ALIGNAS(8)


cc: @sdmaclea

I am not sure why alignment to 8 is needed. I would rather expect DAC_ALIGNAS(IMetaModelCommonRO).

In src/coreclr/md/inc/VerifyLayouts.inc there's a 8byte alignment check for m_Schema:

BEGIN_TYPE(CMiniMdBase, 8) //vtable ptr and first field 8-byte alignment ALIGN_FIELD(CMiniMdBase, m_Schema, sizeof(CMiniMdSchema), 8)

This check started to fail after recent changes to CMiniMdBase and CMiniMdSchema, where new class fields were added. So, I've added explicit 8 byte alignment

src/coreclr/vm/jithelpers.cpp

trylek · 2021-05-10T20:28:12Z

In general this looks reasonable to me. Adding @janvorli as one of the few people familiar with our pre-existing runtime support for Linux x86. Once this PR has been resolved against the current runtime main and demonstrated passing PR testing, I believe it to be ready to be merged in.

gbalykov · 2021-05-11T11:14:51Z

Rebased with some additional changes after #49906

janvorli · 2021-05-11T12:49:47Z

src/coreclr/vm/callingconvention.h

                        }	
-                        case ELEMENT_TYPE_PTR:	
+                        case ELEMENT_TYPE_PTR:
+                            FALLTHROUGH;


I don't think these need the FALLTHROUGH, call you please remove them here?

Thanks, I'll remove this

janvorli · 2021-05-11T12:57:05Z

src/coreclr/vm/field.h

-        SetObjectReference(pObjRef, objRef);
-        GCPROTECT_END();
-    }
+    VOID SetStaticOBJECTREF(OBJECTREF objRef);


What is the reason for moving this method out of the header?

This is also related to access to FieldDesc on linux x86 in IsArgumentInRegister. Otherwise, there are build errors with SetStaticOBJECTREF defined in header, since now "field.h" is included in "common.h"

janvorli · 2021-05-11T13:04:26Z

src/coreclr/vm/jithelpers.cpp

@@ -5415,7 +5415,7 @@ NOINLINE static void JIT_ReversePInvokeEnterRare2(ReversePInvokeFrame* frame, vo
 // As a result, we specially decorate this method to have the correct calling convention
 // and argument ordering for an HCALL, but we don't use the HCALL macros and contracts
 // since this method doesn't follow the contracts.
-void F_CALL_CONV HCCALL3(JIT_ReversePInvokeEnterTrackTransitions, ReversePInvokeFrame* frame, CORINFO_METHOD_HANDLE handle, void* secretArg)
+HCIMPL3_RAW(void, JIT_ReversePInvokeEnterTrackTransitions, ReversePInvokeFrame* frame, CORINFO_METHOD_HANDLE handle, void* secretArg)


I don't understand the reason for switching some of the helpers calling convention. Can you please explain the purpose of these changes?

Calling convention doesn't change here. Actual problem is incorrect usage of HCCALL3 macro: it should be used to call function. HCIMPL3_RAW should be used for declaration instead.

janvorli · 2021-05-11T13:06:01Z

src/coreclr/vm/common.h

@@ -367,6 +367,7 @@ inline void ClrRestoreNonvolatileContext(PCONTEXT ContextRecord)
 #include "pefile.inl"
 #include "excep.h"
 #include "method.hpp"
+#include "field.h"


Which part of the change needs to have the field.h include here?

This is required on linux x86, because IsArgumentInRegister accesses FieldDesc. Otherwise:

/home/z/Dev/runtime3/src/coreclr/binder/../vm/callingconvention.h:416:46: error: member access into incomplete type 'FieldDesc' CorElementType type = pFD->GetFieldType();

Can you please instead move the #include "frames.h" above the #include "field.h" and revert the change in field.h and field.cpp? I've verified it compiles fine on x86 Linux that way.

I still get the original build error this way, because "frames.h" itself includes "callingconvention.h", which requires "field.h" on x86. So, if "frames.h" is above "field.h" there still is error: member access into incomplete type 'FieldDesc'

Ah, I am sorry, I've found I've forgotten to add "cross" to my command line when trying to build for x86. Please keep it as you had it.

janvorli · 2021-05-11T13:06:54Z

src/coreclr/ToolBox/superpmi/superpmi-shim-collector/superpmi-shim-collector.cpp

@@ -153,7 +153,7 @@ extern "C"
    return TRUE;
 }

-extern "C" DLLEXPORT void __stdcall jitStartup(ICorJitHost* host)
+extern "C" DLLEXPORT void jitStartup(ICorJitHost* host)


@BruceForstall is the __stdcall removal ok?

Yes, it is ok. We preffer to just use default calling convention whenever possible.

janvorli

LGTM, thank you!

gbalykov · 2021-06-16T18:26:04Z

Can this be merged?

janvorli · 2021-06-16T19:12:45Z

@gbalykov the libraries tests are failing on Windows x86 in this PR, it might be related. Let me re-run them to see.

gbalykov · 2021-06-18T12:11:12Z

@janvorli I've rebased on latest master, test fails don't seem to be related, am I right?

janvorli · 2021-06-18T12:38:13Z

@gbalykov right, they are in unrelated.

gbalykov · 2021-06-18T12:39:27Z

Thanks!

ghost added the area-Infrastructure-coreclr label Apr 7, 2021

jkoritzinsky previously requested changes Apr 7, 2021

View reviewed changes

src/coreclr/vm/jithelpers.cpp Outdated Show resolved Hide resolved

src/coreclr/vm/jithelpers.cpp Outdated Show resolved Hide resolved

src/coreclr/vm/jithelpers.cpp Outdated Show resolved Hide resolved

gbalykov force-pushed the fix-linux-x86-build branch from 255716d to 70ae805 Compare April 7, 2021 19:35

hoyosjs reviewed Apr 7, 2021

View reviewed changes

jkoritzinsky reviewed Apr 7, 2021

View reviewed changes

src/coreclr/vm/jithelpers.cpp Outdated Show resolved Hide resolved

jkoritzinsky reviewed Apr 7, 2021

View reviewed changes

src/coreclr/vm/jithelpers.cpp Outdated Show resolved Hide resolved

gbalykov force-pushed the fix-linux-x86-build branch from 70ae805 to c6ae5b0 Compare April 7, 2021 21:43

This was referenced Apr 8, 2021

Runtime jobs being abandoned due to infra #50746

Closed

Runtime jobs being abandoned due to infra #50747

Closed

gbalykov force-pushed the fix-linux-x86-build branch 2 times, most recently from 2c60c01 to 2f475ef Compare May 11, 2021 11:13

janvorli reviewed May 11, 2021

View reviewed changes

gbalykov force-pushed the fix-linux-x86-build branch from 2f475ef to 5fac123 Compare May 11, 2021 13:53

gbalykov force-pushed the fix-linux-x86-build branch from 5fac123 to 37717f6 Compare May 19, 2021 10:52

janvorli approved these changes May 19, 2021

View reviewed changes

janvorli closed this May 21, 2021

janvorli reopened this May 21, 2021

gbalykov force-pushed the fix-linux-x86-build branch from 37717f6 to 218272b Compare June 17, 2021 19:26

Fix Linux x86 build

6c63f03

gbalykov force-pushed the fix-linux-x86-build branch from 218272b to 6c63f03 Compare June 18, 2021 08:44

janvorli merged commit f37f0b9 into dotnet:main Jun 18, 2021

ghost locked as resolved and limited conversation to collaborators Jul 18, 2021

		@@ -594,6 +594,7 @@ class CMiniMdBase : public IMetaModelCommonRO


		protected:
		DAC_ALIGNAS(8)

Fix Linux x86 build #50836

Fix Linux x86 build #50836

Uh oh!

Conversation

gbalykov commented Apr 7, 2021

Uh oh!

ghost commented Apr 7, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

trylek commented May 10, 2021

Uh oh!

gbalykov commented May 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janvorli left a comment

Choose a reason for hiding this comment

Uh oh!

gbalykov commented Jun 16, 2021

Uh oh!

janvorli commented Jun 16, 2021

Uh oh!

gbalykov commented Jun 18, 2021

Uh oh!

janvorli commented Jun 18, 2021

Uh oh!

gbalykov commented Jun 18, 2021

Uh oh!

Uh oh!