Skip to content

insert_reachable_cti: on arm32, use the lower order bit in the given …#2457

Open
aleden wants to merge 3 commits intoDynamoRIO:masterfrom
aleden:master
Open

insert_reachable_cti: on arm32, use the lower order bit in the given …#2457
aleden wants to merge 3 commits intoDynamoRIO:masterfrom
aleden:master

Conversation

@aleden
Copy link
Copy Markdown

@aleden aleden commented May 31, 2017

…address instead of assuming that the target is the same as the current ISA mode. clean calls to ARM code, for example, should be allowed.

…address instead of assuming that the target is the same as the current ISA mode. clean calls to ARM code, for example, should be allowed.
@derekbruening
Copy link
Copy Markdown
Contributor

Please see https://github.com/DynamoRIO/dynamorio/wiki/Code-Reviews#commit-messages (the commit message should have a separate title from its body and should capitalize complete sentences in the body).

ASSERT(scratch != REG_NULL); /* required */
/* load target into scratch register */
insert_mov_immed_ptrsz(dcontext, (ptr_int_t)
PC_AS_JMP_TGT(dr_get_isa_mode(dcontext), target),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What I'm puzzling over is the reason that 850c10c put this here: it is called out in the commit message so there must have been something that did not have the lsb set. Not everything does: e.g., fragment tags deliberately do not have lsb set. There must have been something like that that would make its way here. Does a test suite run on arm hit any discrepancy here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think for clean calls, we can rely on the ARM/Thumb bit being set properly in the address of the function. But I am not sure if that is true for the code DynamoRIO emits itself? Then we would need to set it explicitly when moving the address to a register.

@fhahn
Copy link
Copy Markdown
Contributor

fhahn commented Mar 27, 2018

run aarch32 tests

1 similar comment
@fhahn
Copy link
Copy Markdown
Contributor

fhahn commented Mar 27, 2018

run aarch32 tests

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.

3 participants