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

[RISC-V] Add g_GCShadowEnd to JIT_WriteBarrier_Table #90036

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

alpencolt
Copy link

@alpencolt alpencolt commented Aug 4, 2023

lla pseudo instruction which used for access to g_GCShadowEnd in JIT_WriteBarrier implemented via PC-relative addressing. But when W^X enabled, copy of JIT_WriteBarrier is being created and this PC-related addressing is not valid, which cause crash.

This change moves address of g_GCShadowEnd to JIT_WriteBarrier_Table like others variables used in Write Barrier.

Part of #84834
cc @jakobbotsch @Maoni0 @wscho77 @HJLeee @JongHeonChoi @t-mustafin @clamp03 @gbalykov @tomeksowi

lla pseudo instruction which used for access to g_GCShadowEnd
in JIT_WriteBarrier implemented via PC-relative addressing. But when
W^X enabled, copy of JIT_WriteBarrier is being created and this PC-related
addressing is not valid, which cause crash.

This change moves address of g_GCShadowEnd to JIT_WriteBarrier_Table like
others variables used in Write Barrier.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 4, 2023
@alpencolt alpencolt marked this pull request as draft August 4, 2023 17:33
@alpencolt alpencolt force-pushed the risc-v-g_GCShadowEnd branch from a342b99 to 649b3f6 Compare August 4, 2023 17:33
@alpencolt
Copy link
Author

The similar issue should be in ARM64 since there is the same access to g_GCShadowEnd , while PREPARE_EXTERNAL_VAR expands to adrp which is also PC-relative.

@jkotas @Maoni0 JIT_UpdateWriteBarrierState puts new state to set of registers, e.g. for ARM64 it's x0-x7. But I can't find where they used after that. Are they used?

@am11 am11 added the arch-riscv Related to the RISC-V architecture label Aug 5, 2023
@jkotas
Copy link
Member

jkotas commented Aug 6, 2023

JIT_UpdateWriteBarrierState puts new state to set of registers, e.g. for ARM64 it's x0-x7. But I can't find where they used after that. Are they used?

The registers are stored into the local literal pool here: JIT_UpdateWriteBarrierState puts new state to set of registers, e.g. for ARM64 it's x0-x7

@jkotas
Copy link
Member

jkotas commented Aug 6, 2023

The similar issue should be in ARM64 since there is the same access to g_GCShadowEnd , while PREPARE_EXTERNAL_VAR expands to adrp which is also PC-relative.

I agree.

@alpencolt
Copy link
Author

The registers are stored into the local literal pool here: JIT_UpdateWriteBarrierState puts new state to set of registers, e.g. for ARM64 it's x0-x7

Right, I've added g_GCShadowEnd to pool for RISC-V. But comment says (e.g. for ARM64):

    // x0-x7 will contain intended new state
    // x8 will preserve skipEphemeralCheck
    // x12 will be used for pointers

But do these x0-x7 used after that? I can't find it. Probably we could avoid copying of state to x0-x7 registers, if they aren't used at all.

@jkotas
Copy link
Member

jkotas commented Aug 6, 2023

The registers are stored into the literal pool at the end of JIT_UpdateWriteBarrierState: https://github.com/dotnet/runtime/blob/main/src/coreclr/vm/arm64/asmhelpers.S#L258-L261

Are you suggesting that the method can always load a value into a register and then immediately store it into the literal pool? Yes, the code can be written that way, but I do not see anything wrong with how it is written today.

@alpencolt alpencolt marked this pull request as ready for review August 7, 2023 16:43
@jkotas
Copy link
Member

jkotas commented Aug 8, 2023

Do you plan to do matching fix in the arm64 version as well?

@alpencolt
Copy link
Author

Do you plan to do matching fix in the arm64 version as well?

Yes, I'll check on ARM64 environment and make separate PR

@jkotas
Copy link
Member

jkotas commented Aug 8, 2023

Yes, I'll check on ARM64 environment and make separate PR

Thank you!

@jkotas jkotas merged commit 38014ae into dotnet:main Aug 8, 2023
alpencolt pushed a commit to alpencolt/runtime that referenced this pull request Aug 30, 2023
This change moves address of g_GCShadowEnd to JIT_WriteBarrier_Table like
others variables used in Write Barrier.

This fix simmilar to RISC-V one dotnet#90036
jkotas added a commit that referenced this pull request Sep 1, 2023
* [ARM64] Add g_GCShadowEnd to JIT_WriteBarrier_Table

This change moves address of g_GCShadowEnd to JIT_WriteBarrier_Table like
others variables used in Write Barrier.

This fix simmilar to RISC-V one #90036

* [ARM64] Move GCShadow vars to the end of the wbs block

* Update src/coreclr/vm/arm64/asmhelpers.asm

---------

Co-authored-by: Jan Kotas <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-riscv Related to the RISC-V architecture area-VM-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants