Fix x86 linux tests #57244

t-mustafin · 2021-08-11T23:28:46Z

This PR contains commits which fixes build and runtime tests bugs on linux x86 platform. A majority of them fixes calling convention issues.

t-mustafin · 2021-08-12T13:31:18Z

cc @alpencolt @gbalykov

t-mustafin · 2021-08-24T13:19:41Z

cc @jkotas @janvorli

jkotas · 2021-08-24T13:30:52Z

src/coreclr/vm/i386/umthunkstub.S

+#define STACK_ALIGN_MASK 0xf
+    mov     ebx, esp
+    sub     ebx, 4
+    and     ebx, STACK_ALIGN_MASK


This should not be needed. The caller should guarantee the right alignment.

Yes, that patch don't resolve problem entirely. Question about original problem #58191.

Thanks, ebx-method was simple to implement.

jkotas · 2021-08-27T18:41:52Z

@dotnet/jit-contrib Please take a look at the JIT changes that are bulk of this delta. The non-JIT changes LGTM.

src/coreclr/jit/importer.cpp

src/coreclr/jit/morph.cpp

jakobbotsch · 2021-08-28T00:13:47Z

src/coreclr/vm/i386/jithelp.S

+// Place JIT_WriteBarrierEAX address into stack to call it via ret instuction.
+    push eax
+    mov eax, dword ptr [C_FUNC(JIT_WriteBarrierEAX_Loc)]
    xchg    eax, dword ptr [esp]
+// Real return address is still on the stack at esp + 4.
    ret


Are we sure that the above removed instructions are not trying to keep the hardware's call-ret stack balanced?

If no IP-relative computation is needed I think the entire function can just be

mov eax, edx mov edx, ecx jmp dword ptr [C_FUNC(JIT_WriteBarrierEAX_Loc)]

Are we sure that the above removed instructions are not trying to keep the hardware's call-ret stack balanced?

Maybe I do not clearly understand the question. I am sure that the above removed instructions keep the hardware's call-ret info on stack balanced. Removed instructions just calculate wrong address.

If no IP-relative computation is needed I think the entire function can just be

mov eax, edx mov edx, ecx jmp dword ptr [C_FUNC(JIT_WriteBarrierEAX_Loc)]

Unfortunately this code jumps to JIT_WriteBarrierEAX_Loc, not to the address stored in JIT_WriteBarrierEAX_Loc:

=> 0xf7606c71 <JIT_WriteBarrier_Callable>: mov eax,edx 0xf7606c73 <JIT_WriteBarrier_Callable+2>: mov edx,ecx 0xf7606c75 <JIT_WriteBarrier_Callable+4>: jmp 0xf7ad49b4 gdb$ disassemble 0xf7ad49b4 Dump of assembler code for function JIT_WriteBarrierEAX_Loc: 0xf7ad49b4: enter 0x6075,0xf7 End of assembler dump. gdb$ disassemble 0xf76075c8 Dump of assembler code for function JIT_WriteBarrierEAX: 0xf76075c8 <+0>: mov DWORD PTR [edx],eax 0xf76075ca <+2>: cmp eax,0xf1a0000c 0xf76075d0 <+8>: jb 0xf76075df <JIT_WriteBarrierEAX+23> 0xf76075d2 <+10>: shr edx,0xa 0xf76075d5 <+13>: nop 0xf76075d6 <+14>: cmp BYTE PTR [edx-0xa4007dc],0xff 0xf76075dd <+21>: jne 0xf76075e2 <JIT_WriteBarrierEAX+26> 0xf76075df <+23>: ret 0xf76075e0 <+24>: nop 0xf76075e1 <+25>: nop 0xf76075e2 <+26>: mov BYTE PTR [edx-0xa4007dc],0xff 0xf76075e9 <+33>: ret

Maybe I do not clearly understand the question. I am sure that the above removed instructions keep the hardware's call-ret info on stack balanced. Removed instructions just calculate wrong address.

The hardware tracks call/ret pairs to be able to branch predict ret instructions better. It means that after this change the hardware might mispredict a lot of the upcoming ret instructions.

Unfortunately this code jumps to JIT_WriteBarrierEAX_Loc, not to the address stored in JIT_WriteBarrierEAX_Loc:

That's odd. Maybe try AT&T syntax? It should assemble to ff 25 imm32.

I can reproduce it with clang-9 -m32 foo.S locally. Using AT&T syntax instead works and assembles correctly:

.att_syntax jmpl *C_FUNC(JIT_WriteBarrierEAX_Loc)

Hmm, with GOT we do not need JIT_WriteBarrierEAX_Loc at all. The code below works properly:

mov eax, edx mov edx, ecx push eax call 1f 1: pop eax 2: .att_syntax addl $_GLOBAL_OFFSET_TABLE_+(2b-1b), %eax .intel_syntax noprefix mov eax, dword ptr [eax + C_FUNC(JIT_WriteBarrierEAX)@GOT] xchg eax, dword ptr [esp] ret

I don't see how this would work with the W^X enabled. We don't want to call the JIT_WriteBarrierEAX in the executable, but rather its copy that is placed at a random memory location (in a dynamically allocated memory page).

I don't see how this would work with the W^X enabled. We don't want to call the JIT_WriteBarrierEAX in the executable, but rather its copy that is placed at a random memory location (in a dynamically allocated memory page).

Would current PR variant with JIT_WriteBarrierEAX_Loc dereferencing work with W^X enabled?

Yes, if that works with W^X disabled, it would work with it enabled too. The JIT_WriteBarrierEAX_Loc points to the JIT_WriteBarrierEAX with W^X disabled and to its copy with W^X enabled.

Nice, JIT_WriteBarrierEAX_Callable is called in every simple program and now it works with current PR on x86 linux.

src/coreclr/inc/corinfo.h

src/coreclr/jit/reglist.h

src/coreclr/vm/i386/asmhelpers.S

src/coreclr/vm/i386/asmhelpers.asm

[unix x86] Fix tests build [unix x86] Add register map for crossgen2 Fixes readytorun/coreroot_determinism/coreroot_determinism/coreroot_determinism.sh on x86 linux [unix x86] Fix tail calls tests [unix x86] Fix unmanaged callconv [unix x86] Fix passing implicit args via stack [unix x86] Pop hidden retbuff arg on cdecl callconv [unix x86] Fix WriteBarrier call [x86] Add calling convention name print to dump [x86] Use ebx to pass VASigCookie to GenericPInvokeCalliHelper It fixes stack alignment in GenericPInvokeCalliHelper on unix x86 Fix storageType overflow assertion in TinyArray

ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 11, 2021

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 11, 2021

t-mustafin force-pushed the Fix_x86_linux_tests_build branch from 8ce1079 to 6dd78be Compare August 12, 2021 12:33

t-mustafin force-pushed the Fix_x86_linux_tests_build branch 3 times, most recently from eb31d7b to ea6200e Compare August 18, 2021 04:22

runfoapp bot mentioned this pull request Aug 18, 2021

Test failure readytorun/determinism/crossgen2determinism/crossgen2determinism.sh #57620

Closed

t-mustafin force-pushed the Fix_x86_linux_tests_build branch from 052bdae to 100a75b Compare August 23, 2021 20:43

t-mustafin marked this pull request as ready for review August 24, 2021 13:14

jkotas reviewed Aug 24, 2021

View reviewed changes

t-mustafin force-pushed the Fix_x86_linux_tests_build branch from 100a75b to fcb94eb Compare August 27, 2021 16:41

t-mustafin requested a review from MichalStrehovsky as a code owner August 27, 2021 16:41

jakobbotsch reviewed Aug 28, 2021

View reviewed changes

src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved

jakobbotsch reviewed Aug 28, 2021

View reviewed changes

src/coreclr/jit/morph.cpp Show resolved Hide resolved

jakobbotsch reviewed Aug 28, 2021

View reviewed changes

BruceForstall requested changes Aug 28, 2021

View reviewed changes

src/coreclr/inc/corinfo.h Outdated Show resolved Hide resolved

src/coreclr/jit/reglist.h Show resolved Hide resolved

t-mustafin commented Aug 30, 2021

View reviewed changes

src/coreclr/vm/i386/asmhelpers.S Outdated Show resolved Hide resolved

src/coreclr/vm/i386/asmhelpers.asm Outdated Show resolved Hide resolved

t-mustafin requested a review from BruceForstall August 30, 2021 23:40

t-mustafin force-pushed the Fix_x86_linux_tests_build branch from ae3c64f to 8a1bae9 Compare August 31, 2021 15:21

jakobbotsch approved these changes Aug 31, 2021

View reviewed changes

jkotas approved these changes Aug 31, 2021

View reviewed changes

t-mustafin force-pushed the Fix_x86_linux_tests_build branch from 8a1bae9 to 84f81ba Compare September 3, 2021 15:28

t-mustafin force-pushed the Fix_x86_linux_tests_build branch from 84f81ba to caa59b9 Compare September 3, 2021 15:31

t-mustafin requested a review from janvorli September 3, 2021 15:35

BruceForstall approved these changes Sep 18, 2021

View reviewed changes

BruceForstall merged commit 7263ce5 into dotnet:main Sep 18, 2021

t-mustafin deleted the Fix_x86_linux_tests_build branch September 20, 2021 07:36

ghost locked as resolved and limited conversation to collaborators Nov 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix x86 linux tests #57244

Fix x86 linux tests #57244

t-mustafin commented Aug 11, 2021

t-mustafin commented Aug 12, 2021

t-mustafin commented Aug 24, 2021

jkotas Aug 24, 2021 •

edited

Loading

t-mustafin Aug 26, 2021 •

edited

Loading

t-mustafin Aug 27, 2021

jkotas commented Aug 27, 2021

jakobbotsch Aug 28, 2021

jakobbotsch Aug 28, 2021

t-mustafin Aug 30, 2021

jakobbotsch Aug 30, 2021

jakobbotsch Aug 30, 2021 •

edited

Loading

t-mustafin Sep 3, 2021

janvorli Sep 3, 2021

t-mustafin Sep 3, 2021

janvorli Sep 3, 2021

t-mustafin Sep 3, 2021

Fix x86 linux tests #57244

Fix x86 linux tests #57244

Conversation

t-mustafin commented Aug 11, 2021

t-mustafin commented Aug 12, 2021

t-mustafin commented Aug 24, 2021

jkotas Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

t-mustafin Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Aug 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch Aug 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Aug 24, 2021 •

edited

Loading

t-mustafin Aug 26, 2021 •

edited

Loading

jakobbotsch Aug 30, 2021 •

edited

Loading