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.

Sign up for GitHub

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

Jump to bottom

Incorrect function return address for tailcall when -record_replace_retaddr flag is used. #6394

Closed
ivankyluk opened this issue Oct 25, 2023 · 0 comments · Fixed by #6395
Closed

Incorrect function return address for tailcall when -record_replace_retaddr flag is used. #6394

ivankyluk opened this issue Oct 25, 2023 · 0 comments · Fixed by #6395
Assignees
@ivankyluk

Comments

@ivankyluk
Copy link
Contributor

When drcachesim is used with -offline, -record_replace_retaddr, -record_function, it leaves sentinels in the trace when a tailcall to a function which is being recorded is encountered.

The issue can be reproduced with common.getretaddr under build_suite as follows:

./build/bin64/drrun -t drcachesim -offline -record_replace_retaddr -record_function 'tailcall_with_retaddr|1&foo|1' -- ../build_suite/build_debug-internal-64/suite/tests/bin/common.getretaddr

<marker: function #0>
<marker: function return address PC>
<marker: function argument ARG1>

<marker: function #1>
<marker: function return address SENTINEL>
<marker: function argument ARG2>

The return address of function #1 should be PC, the same as function #0.
The current implementation leaves the SENTINEL in the trace instead.
@ivankyluk ivankyluk self-assigned this Oct 25, 2023
@ivankyluk ivankyluk mentioned this issue Oct 25, 2023
Merged
ivankyluk added a commit that referenced this issue Oct 27, 2023
@ivankyluk
i#6394: Replace sentinel in trace with real return address. (#6395)
46d4849 
Update drwrap to check if the return address is a sentinel. If it is,
look for the real return address at the outer level.

When -record_replace_retaddr is used in X86 platforms, the return
address in the stack is replaced by the sentinel.
For a tailcall, the current implementation uses the address in the
stack, the sentinel, as the return address. The change is to check if
the return address in the stack is the sentinel or not. If it is,
replace it with the return address of the outer level.

The problem can be reproduced by 

./build/bin64/drrun -t drcachesim -offline -record_replace_retaddr
-record_function 'tailcall_with_retaddr|1&foo|1' --
../build_suite/build_debug-internal-64/suite/tests/bin/common.getretaddr

A test is added to cover -record_replace_retaddr with common.getretaddr
which has a tailcall.

Fixes: #6394
@ivankyluk ivankyluk mentioned this issue Feb 21, 2024
Closed
@derekbruening derekbruening mentioned this issue Feb 21, 2024
Merged
ivankyluk added a commit that referenced this issue Feb 22, 2024
@ivankyluk
i#6668: Replace drwrap retaddr sentinel on all architectures, not jus…
6a06ccb 
…t x86 (#6670)

Expand drwrap's return address sentinel replacement from just x86 (as
was added in PR #6395) to cover all architectures.

When -record_replace_retaddr is used in ARM (as well as x86) platforms,
the return address in the stack is replaced by the sentinel.
For a tailcall, the current implementation uses the address in the
stack, the sentinel, as the return address. The change is to check if
the return address in the stack is the sentinel or not. If it is,
replace it with the return address of the outer level.

The problem can be reproduced by

./build/bin64/drrun -t drcachesim -offline -record_replace_retaddr
-record_function 'tailcall_with_retaddr|1&foo|1' --
../build_suite/build_debug-internal-64/suite/tests/bin/common.getretaddr

on an ARM machine.

This change expends PR #6395  to cover ARM.

Issues: #6394, #6668
Fixes: #6668
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ivankyluk ivankyluk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

i#6394: Replace sentinel in trace with real return address. DynamoRIO/dynamorio
1 participant
@ivankyluk