Record and replay architected timer accesses on arm64 #3852
Conversation
|
The new tests will need to be skipped on arm64 if the prctl is not supported; that's what's causing the CI failure. |
| @@ -230,7 +230,7 @@ DiversionSession::DiversionResult DiversionSession::diversion_step( | |||
| auto special_instruction = special_instruction_at(t, t->ip()); | |||
| if (special_instruction.opcode == SpecialInstOpcode::X86_RDTSC) { | |||
| size_t len = special_instruction_len(special_instruction.opcode); | |||
| uint64_t rdtsc_value = next_rdtsc_value(); | |||
| uint64_t rdtsc_value = next_timer_counter(); | |||
There was a problem hiding this comment.
Normally for a change of this size it would be better to split out the renaming into its own commit, but I don't think we need to do this now.
| @@ -1114,6 +1114,14 @@ void Task::post_exec_syscall(const std::string& original_exe_file) { | |||
| remote.infallible_syscall(syscall_number_for_arch_prctl(arch()), | |||
| ARCH_SET_CPUID, 0); | |||
| } | |||
| if (arch() == aarch64) { | |||
| if (remote.syscall(syscall_number_for_prctl(remote.task()->arch()), | |||
| PR_SET_TSC, PR_TSC_SIGSEGV, 0, 0) != 0) { | |||
There was a problem hiding this comment.
Why aren't we doing this from set_up_process_arch() in Task.cc where we do it for x86?
And I think we need to change Task::reenable_cpuid_tsc() to work for ARM too.
There was a problem hiding this comment.
Why aren't we doing this from set_up_process_arch() in Task.cc where we do it for x86?
That's because the arm64 prctl does not preserve this state across an execve.
And I think we need to change Task::reenable_cpuid_tsc() to work for ARM too.
Done.
There was a problem hiding this comment.
That's because the arm64 prctl does not preserve this state across an execve.
Is this intentionally different from x86? Because that difference sounds very unfortunate :-(
There was a problem hiding this comment.
Yeah, it would have been difficult to follow x86 here because PR_SET_TSC on arm64 is 64-bit only and it's not clear what should happen when a 64-bit process execs a 32-bit process which then execs a 64-bit process.
I think of the x86 behavior as a mistake (which was corrected when ARCH_SET_CPUID was added), PR_SET_TSC is really a property of the code running in a process and not necessarily that of the child processes.
| void breakpoint(void) {} | ||
|
|
||
| int main(void) { | ||
| initial_cntfrq = cntfrq(); |
There was a problem hiding this comment.
I think this test should detect whether the kernel supports PR_SET_TSC.
One way to do that would be to give it a command-line option (something simple like "any arguments at all") that checks whether PR_SET_TSC is EINVAL and returns an exit code indicating that. Then we can call it (not under rr) from arch_timer.run and skip the test if that PR_SET_TSC is not supported.
Another way to do it would be in record_syscall.cc --- make PR_SET_TSC return EINVAL if the underlying kernel doesn't support it. Then the test can call PR_SET_TSC and bail out with EXIT-SUCCESS if it gets EINVAL. This would be good... currently when running under rr PR_SET_TSC claims to work even if it actually doesn't.
There was a problem hiding this comment.
Let's go with the first option. We've already had to accept that if prctl(PR_SET_TSC) isn't implemented in the kernel, things will be slightly broken, and prctl(PR_SET_TSC) not functioning correctly inside the recorded process is probably less of a problem than the nondeterministic timers.
Since we need to skip two tests (arm/arch_timer and prctl_tsc) , I decided to do the test as part of a separate program that is called by both tests' .run scripts.
In the process of doing so I discovered that I had added the new test to the wrong list so it wasn't actually running the .run script (which turned out to have a bug which meant that it would always fail), so I fixed that as well.
This works using the recently added support for prctl(PR_SET_TSC) on arm64 which is due to be released in kernel version 6.12. Fixes rr-debugger#3740
|
Thanks! |
This works using the recently added support for prctl(PR_SET_TSC) on arm64 which is due to be released in kernel version 6.12.
Fixes #3740