i#4128: Add cleancall reg read/write flags #5164
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds new dr_cleancall_save_t flags which are required for proper interaction between clean calls and drreg: DR_CLEANCALL_READS_APP_CONTEXT must be set for dr_get_mcontext() to obtain the proper values, and #DR_CLEANCALL_WRITES_APP_CONTEXT must be set to ensure that dr_set_mcontext() is persistent. Adds a clean call insertion event to enable drreg to know about clean calls at the time they are inserted. dr_insert_clean_call_ex() invokes the callback and passes the flags to drreg, who then treats the clean call as an app instruction. For annotations, for now we leave drreg looking for the annotation label (possible future changes #5160 or #5161 would eliminate this special case). dr_insert_{cbr,ubr,mbr,call}_instrumentation() always set both labels. drwrap always sets both labels for pre and post callbacks. Updates uses throughout our tests and samples to use the new flags as appropriate. Adds a new dedicated test client.drwrap-drreg-test which tests both a drwrap call and a direct clean call. Fixes a missing drwrap cache invalidation on module unload that the new test uncovers. Fixes #4128
|
Unfortunately it is looking like #4711 is reproducing |
…dd drreg_statelessly_restore_all()
|
x86 linux.signal0001 test failed with output ordering differences: filed #5167 Looking at the a64 failures |
abhinav92003
approved these changes
Oct 19, 2021
derekbruening
commented
Oct 19, 2021
derekbruening
commented
Oct 19, 2021
+ Review requests are mostly augmenting comments and adding some error reporting. + Fixes a bug in where the clean call is inserted. Confirmed the improved test fails w/o this fix. + Fixes a bug in where_respill. Confirmed the improved test fails w/o this fix. + Fixes a pre-existing drreg bug in drreg_spill_aflags() which set xax ever_spilled to true even if it wasn't spilled b/c it was dead. + Improves the reg_val clean call test, and adds a new test of MULTIPATH.
|
I'm hitting failures coming from the bug fix in restoring xax when it's dead. That now fails, which it should do since xax is dead and so we can't actually get the app value. I'm changing the 7 drreg test asm sequence to have xax be live, so this sort of hack to get the flags out of xax still works. |
only: we have to allow DR to invoke the callback during mangling.
Fix drreg-test cases that now have failures coming from the bug fix in
restoring xax when it's dead.
drreg-test has cases like Test 21 where xax is dead but it does this:
/* Make sure that aflags are spilled to some slot, instead of being stored in xax.
*/
CHECK(drreg_get_app_value(drcontext, bb, inst, DR_REG_XAX, DR_REG_XCX) ==
DRREG_SUCCESS,
"cannot get app value");
That now fails, which it should do since xax is dead and so we can't
actually get the app value.
I changed the 7 drreg test asm sequence to have xax be live, so this
sort of hack to get the flags out of xax still works.
abhinav92003
approved these changes
Oct 20, 2021
derekbruening
added a commit
that referenced
this pull request
Oct 20, 2021
Fixes a drreg bug in drreg_spill_aflags() which set xax's ever_spilled flag to true even if it wasn't spilled because it was dead. This caused requests to get the app value of xax to return success and to move aflags out of xax and into a slot, but without restoring the app value. The fixed behavior is to fail up front if xax is dead and so leave aflags in xax. Updates the drreg tests that rely on getting the xax value moving aflags into a slot to use a live xax rather than a dead xax. This was discovered while adding PR #5164 for #4128. Issue: #4128
|
I split the ever_spilled fix off as PR #5170 to isolate it and make it easier to attribute any future problems to that change versus this big clean call change. |
derekbruening
added a commit
that referenced
this pull request
Oct 20, 2021
Fixes a drreg bug in drreg_spill_aflags() which set xax's ever_spilled flag to true even if it wasn't spilled because it was dead. This caused requests to get the app value of xax to return success and to move aflags out of xax and into a slot, but without restoring the app value. The fixed behavior is to fail up front if xax is dead and so leave aflags in xax. Updates the drreg tests that rely on getting the xax value moving aflags into a slot to use a live xax rather than a dead xax. This was discovered while adding PR #5164 for #4128. Issue: #4128
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds new dr_cleancall_save_t flags which are required for proper
interaction between clean calls and drreg:
DR_CLEANCALL_READS_APP_CONTEXT must be set for dr_get_mcontext() to
obtain the proper values, and #DR_CLEANCALL_WRITES_APP_CONTEXT must be
set to ensure that dr_set_mcontext() is persistent.
Adds a clean call insertion event to enable drreg to know about clean
calls at the time they are inserted. dr_insert_clean_call_ex()
invokes the callback and passes the flags to drreg, who then treats
the clean call as an app instruction.
For annotations, for now we leave drreg looking for the annotation
label (possible future changes #5160 or #5161 would eliminate this
special case).
dr_insert_{cbr,ubr,mbr,call}_instrumentation() always set both labels.
drwrap always sets both labels for pre and post callbacks.
Updates uses throughout our tests and samples to use the new flags as
appropriate.
Adds a new dedicated test client.drwrap-drreg-test which tests both a
drwrap call and a direct clean call.
Fixes a missing drwrap cache invalidation on module unload that the
new test uncovers.
Fixes #4128