search

DynamoRIO / dynamorio

Dynamic Instrumentation Tool Platform
Other
2.65k stars 560 forks source link

AArchXX clean calls handle far too few use cases, blocking tool development #2210

Open derekbruening opened 7 years ago

derekbruening commented 7 years ago

The AArchXX implementation of insert_parameter_preparation() is missing some key functionality:

/* FIXME i#1551, i#1569: we only implement naive parameter preparation,
 * where args are all regs or immeds and do not conflict with param regs.
 */

This is a big enough missing port feature to call for a separate issue. This is blocking tool development on AArchXX, even simple samples: https://github.com/DynamoRIO/dynamorio/pull/2174

Xref original issues #1551, #1569

derekbruening commented 7 years ago

This shouldn't have been closed as handling memory operands as arguments is not yet finished.

egrimley commented 7 years ago

(Why did it get closed? A bug in GitHub?)

egrimley commented 7 years ago

Memory operands will need a somewhat different approach as there will be cases (even if they occur very rarely in practice) in which it is necessary to spill and restore registers during the marshalling: imagine (31 * 31) arguments of the form [Xn, Xm], for example.

We should start by specifying exactly which types of memory operands should be accepted.

derekbruening commented 7 years ago

(Why did it get closed? A bug in GitHub?)

Your commit message closed it by saying "Fixes #2210"

derekbruening commented 7 years ago

It seems fine to disallow hard-to-handle memory operands if they're unlikely to be used much by clients.

egrimley commented 7 years ago

Really? I can't see those words on this page: https://github.com/DynamoRIO/dynamorio/commit/e06883cbc9515828379109c0ff20695e71b86cc8

derekbruening commented 7 years ago

(Why did it get closed? A bug in GitHub?)

Your commit message closed it by saying "Fixes #2210"

Really? I can't see those words on this page: e06883c

Hmm, right, it's not there in the final squash-and-merge commit, only in the first one in the pull request. Maybe worth filing a github bug.