Skip to content
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

NativeAOT/win/arm64: Fix the reloc type typo #104516

Merged
merged 13 commits into from
Jul 20, 2024

Conversation

kunalspathak
Copy link
Member

@kunalspathak kunalspathak commented Jul 7, 2024

  • There was a typo, where a relocation should be IMAGE_REL_ARM64_SECREL_LOW12A instead of IMAGE_REL_ARM64_SECREL_LOW12L .
  • The codegen was not handling the add operation correctly, so fixed that.
  • Fixed the handling of the new relocs in superpmi.
  • Added Arm64 target in reproNative.vcxproj for developing/testing on arm64.

Fixes: #104500

@kunalspathak
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@VSadov
Copy link
Member

VSadov commented Jul 8, 2024

I looked at your sample of the emitted native code and I think there is also an issue with _tls_index temporarily tracked as GC pointer.
here we are indirectly loading an int32 from _tls_index into w1

adrp  x1, [HIGH RELOC _tls_index]
add   x1, x1, [LOW RELOC _tls_index]
   ; gcrRegs +[x1]			<-- this seems incorrect.  _tls_index is a native datastructure
ldr   w1, [x1]				
   ; gcrRegs -[x1]

x1 should be just native int, not a gc pointer.

also the asm/msvc versions skip the add entirely.
ideally this should be

adrp  x1, [HIGH RELOC _tls_index]
ldr   w1, [x1, LOW RELOC _tls_index]				

@VSadov
Copy link
Member

VSadov commented Jul 8, 2024

The whole thing could be

        adrp        $destReg, [HIGH RELOC _tls_index]
        ldr         $TrashRegister32Bit, [$destReg, LOW RELOC _tls_index]

        ldr         $destReg, [xpr, #__tls_array]
        ldr         $destReg, [$destReg, $TrashRegister32Bit uxtw #3]

        add         $destReg, $destReg, #0, lsl #0xC
        RELOC       0xA, tls_InlinedThreadStatics                          ;; IMAGE_REL_ARM64_SECREL_HIGH12A
        add         $destReg, $destReg, #0, lsl #0
        RELOC       0x9, tls_InlinedThreadStatics                          ;; IMAGE_REL_ARM64_SECREL_LOW12A

Here $destReg is a native pointer to the location where managed pointer to TLS root is stored.
Nothing needs tracking yet.

        ldr         x19, [$destReg]
           ; gcrRegs +[x19]

Here we have a GC pointer to the managed TLS root

@kunalspathak
Copy link
Member Author

Fixed to not track the tls_index.

G_M37313_IG03:        ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, isz
            ldr     x0, [xpr, #0x58]
            adrp    x1, [HIGH RELOC #0x4000000000420060]
            add     x1, x1, [LOW RELOC #0x4000000000420060]
            ldr     w1, [x1]
            ldr     x0, [x0, w1, UXTW #3]
            add     x1, x1, [LOW RELOC #0x4000000000420068]
            add     x1, x1, #0
            add     x0, x0, x1
            ldr     x19, [x0]
                             ; gcrRegs +[x19]
            cbz     x19, G_M37313_IG07

@kunalspathak
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@VSadov
Copy link
Member

VSadov commented Jul 17, 2024

for the

            add     x1, x1, [LOW RELOC #0x4000000000420068]
            add     x1, x1, #0

why the second add has no reloc? Is that just visualization?

@kunalspathak
Copy link
Member Author

for the

            add     x1, x1, [LOW RELOC #0x4000000000420068]
            add     x1, x1, #0

why the second add has no reloc? Is that just visualization?

yes, fixed it and also fixed the codegen. Now we generate this:

G_M37313_IG03:  ;; offset=0x001C
        F9402E40          ldr     x0, [xpr, #0x58]
        90000001          adrp    x1, [HIGH RELOC #0x4000000000420060]
        91000021          add     x1, x1, [LOW RELOC #0x4000000000420060]
        B9400021          ldr     w1, [x1]
        F8615800          ldr     x0, [x0, w1, UXTW #3]
        91400000          add     x0, x0, [HIGH RELOC #0x4000000000420068]
        91000000          add     x0, x0, [LOW RELOC #0]
        F9400013          ldr     x19, [x0]
        B40000F3          cbz     x19, G_M37313_IG07

@kunalspathak
Copy link
Member Author

image

@kunalspathak kunalspathak added arm-sve Work related to arm64 SVE/SVE2 support and removed arm-sve Work related to arm64 SVE/SVE2 support labels Jul 18, 2024
@kunalspathak
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kunalspathak kunalspathak marked this pull request as ready for review July 19, 2024 05:28
@kunalspathak kunalspathak requested a review from VSadov July 19, 2024 05:28
@kunalspathak
Copy link
Member Author

@VSadov @dotnet/jit-contrib PTAL

Comment on lines +2761 to +2762
else if (compiler->IsTargetAbi(CORINFO_NATIVEAOT_ABI) && TargetOS::IsWindows &&
op2->IsIconHandle(GTF_ICON_SECREL_OFFSET))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this constant isn't considered a reloc?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is and hence we add EA_CNS_RELOC_FLG | EA_CNS_SEC_RELOC flags below.

@kunalspathak kunalspathak requested a review from jkotas July 19, 2024 15:10
@kunalspathak
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@VSadov
Copy link
Member

VSadov commented Jul 19, 2024

failures in System.IO.Packaging are likely related to #105160

@VSadov
Copy link
Member

VSadov commented Jul 19, 2024

otherwise windows-arm64 Release does not have any failures

@VSadov
Copy link
Member

VSadov commented Jul 19, 2024

I've looked at emitted code in few places and it looks correct.
A typical case:

S_P_CoreLib_System_Threading_Monitor__Enter:
  0000000140494090: A9BD7BFD  stp         fp,lr,[sp,#-0x30]!
  0000000140494094: A901D3F3  stp         x19,x20,[sp,#0x18]
  0000000140494098: F90017F5  str         x21,[sp,#0x28]
  000000014049409C: 910003FD  mov         fp,sp
  00000001404940A0: F9000BBF  str         xzr,[fp,#0x10]
  00000001404940A4: AA0003F3  mov         x19,x0

  // fetching managed TLS root into x20
  00000001404940A8: F9402E40  ldr         x0,[xpr,#0x58]
  00000001404940AC: F000C4C1  adrp        x1,uenum_close_ptr
  00000001404940B0: 91080021  add         x1,x1,#0x200
  00000001404940B4: B9400021  ldr         w1,[x1]
  00000001404940B8: F8615800  ldr         x0,[x0,w1 uxtw #3]
  00000001404940BC: 91400000  add         x0,x0,#0,lsl #0xC
  00000001404940C0: 91004000  add         x0,x0,#0x10
  00000001404940C4: F9400014  ldr         x20,[x0]

  00000001404940C8: B4000BB4  cbz         x20,000000014049423C
. . .
. . .

Copy link
Member

@VSadov VSadov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!!

@EgorBo
Copy link
Member

EgorBo commented Jul 19, 2024

Sorry, I didn't realize your PR bumped the JIT-EE guid and merged a community PR with jit-ee guid bump. I think you can probably resolve the conflict and merge without waiting for CI..

@kunalspathak
Copy link
Member Author

/ba-g failures are unrelated

@kunalspathak kunalspathak merged commit 922c2d8 into dotnet:main Jul 20, 2024
87 of 94 checks passed
@kunalspathak kunalspathak deleted the aot-diffs branch July 20, 2024 01:17
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NativeAOT] Intermittent crash accessing thread statics on Windows Arm64
4 participants