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

i#3544 RV64: Rebase the dcontext pointer. #7235

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

mariospaok4
Copy link
Contributor

@mariospaok4 mariospaok4 commented Jan 30, 2025

For RISC-V the dcontext_t struct is larger than the biggest valid displacement of the load and store instructions.
By rebasing the pointer kept in spill_state_t's TLS slot by 0x800, we can access the entire struct, because the displacement can be in the range of -0x800 to 0x7ff. Architectures other than RV64, are unaffected by these changes.

The dcontext_t struct is larger than the biggest valid displacement of the load and store instructions.
By rebasing the pointer by 0x800, we van access the entire struct,
because the dispacement can be in the renge of -0x800 to 0x7ff.
@mariospaok4 mariospaok4 marked this pull request as draft January 30, 2025 09:19
@mariospaok4
Copy link
Contributor Author

mariospaok4 commented Feb 7, 2025

I know that these patches are quite "hacky". However, I could not find a better way to work around some issues.
Currently we use only arithmetic, we can use a separate TLS slot for the displaced dcontext, if it is deemed worthwhile.
That being said, only riscv is affected by the patch and it fixes dr_insert_read_tls_field.
We can add sample client that demonstrates this.
It also seems that no previously passing tests fail.

@mariospaok4 mariospaok4 marked this pull request as ready for review February 7, 2025 10:50
core/arch/arch_exports.h Outdated Show resolved Hide resolved
core/dispatch.c Outdated Show resolved Hide resolved
core/unix/os.c Outdated Show resolved Hide resolved
@ziyao233 ziyao233 requested a review from ksco February 8, 2025 08:46
core/arch/arch_exports.h Outdated Show resolved Hide resolved
core/dispatch.c Outdated Show resolved Hide resolved
@ksco ksco requested a review from derekbruening February 10, 2025 05:19
@derekbruening
Copy link
Contributor

@ksco ksco requested a review from derekbruening 16 hours ago

FYI I am out of office today; will take a look tomorrow.

@derekbruening
Copy link
Contributor

1 failing and 18 successful checks

The failure is the sourceware.org network issue. Several flaky issues were fixed in the last several days: probably worth merging the latest from master.

core/arch/arch_exports.h Show resolved Hide resolved
core/arch/arch_exports.h Show resolved Hide resolved
core/arch/arch_exports.h Outdated Show resolved Hide resolved
core/arch/arch_exports.h Outdated Show resolved Hide resolved
core/arch/arch_exports.h Outdated Show resolved Hide resolved
core/arch/mangle_shared.c Outdated Show resolved Hide resolved
@@ -698,12 +698,13 @@ append_restore_simd_reg(dcontext_t *dcontext, instrlist_t *ilist, bool absolute)
*
* ma ta sew=8 lmul=8 */
vtypei = (0b1 << 7) | (0b1 << 6) | (0b000 << 3) | 0b011;
memopnd = opnd_create_dcontext_field_via_reg_sz(
dcontext, DR_REG_A1, 0, reg_get_size_lmul(DR_REG_VR0, RV64_LMUL_8));
memopnd = opnd_create_base_disp(DR_REG_A1, REG_NULL, 0, 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this temporary and you want to go back to the opnd_create_dc* routine? Added a TODO i#3544: ...? Ditto below.

Copy link
Contributor Author

@mariospaok4 mariospaok4 Feb 12, 2025

Choose a reason for hiding this comment

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

To me, it looks like opnd_create_dcontext_field_via_reg_sz() should not be used for this specific operand. We do not want to access a dcontext field by base+offset. The memory address where the the respective memory operation is expected to happen is already in register a1. This worked before, but now it tries to access a1-0x800 instead of a1. So I believe, it should not be temporary.

EDIT: s/respected/respective/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this the only remaining unresolved conversation, we would like some clarification on that. I have stated my opinion that opnd_create_base_disp() is more appropriate in these cases. What are we going to do with it?

core/arch/riscv64/mangle.c Outdated Show resolved Hide resolved
core/unix/os.c Outdated Show resolved Hide resolved
core/arch/arch_exports.h Show resolved Hide resolved
core/arch/arch_exports.h Show resolved Hide resolved
core/arch/riscv64/mangle.c Outdated Show resolved Hide resolved
core/arch/arch_exports.h Outdated Show resolved Hide resolved
core/arch/arch_exports.h Outdated Show resolved Hide resolved
mariospaok4 and others added 5 commits February 13, 2025 11:03
Renamed some macros, added helper macros to replace raw arithmetic operations,
Added missing DCONTEXT_ACTUAL_TO_TLS_OFFSET.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants