Improve "sysnums" test (signal_pre_syscall.c). #7163
Conversation
The test is rewritten to repeatedly adjust the timer so that it hits the critical section more often. On AArch64 without DEBUG the new test ran in about 0.5 s instead of about 15 s and hit the critical section about 400 times instead of about 1 time. Change-Id: I96b51e817b57dddaabf9ece193d333d78085f84c
abhinav92003
requested review from
abhinav92003
and removed request for
derekbruening
| { | ||
| long seconds = 30; | ||
|
|
||
| /* Parse optional "seconds" argument. */ |
There was a problem hiding this comment.
Would it help to use a droption_t instead?
Edit: this seems to be unused actually.
| syscall_wrapper(); | ||
| syscall_wrapper(volatile int *); | ||
|
|
||
| static volatile int flag1; |
There was a problem hiding this comment.
Maybe call them flag_handler_src, flag_handler_dst, or something similar.
| * signal will never appear to have happened during the critical block. | ||
| */ | ||
| int | ||
| try(timer_t timer, unsigned long long ns) |
There was a problem hiding this comment.
s/ns/timer_duration_ns/
| if (unchanged_results < 5) | ||
| step = step / 2 > 0 ? step / 2 : step; | ||
| else | ||
| step = step * 2 > 0 ? step * 2 : step; |
There was a problem hiding this comment.
Can this just be step *= 2? Though I see we strive to ensure step is never zero, but: if step*2 is zero, step must be zero too, so same outcome.
| { | ||
| uint64_t time = 10000000; | ||
| /* Stop trying if the signal arrived during the critical block. | ||
| * This never happens (or seems never to happen) under DynamoRIO. |
There was a problem hiding this comment.
Maybe add an assert result !=0 || !dr_app_running_under_dynamorio()?
| for (int i = 0; i < 1000; ++i) | ||
| syscall_wrapper(); | ||
| } | ||
| unsigned long long ns = 1024; /* arbitrary initial value */ |
There was a problem hiding this comment.
s/ns/timer_duration_ns/
| time -= i * 100000; | ||
| /* Adjust ns for next try. */ | ||
| if (result < 0) | ||
| ns = ns + step > ns ? ns + step : (unsigned long long)-1; |
There was a problem hiding this comment.
When would ns + step > ns be false? I thought we try that step always stays positive?
| fdiv v10.2d, v11.2d, v12.2d | ||
| fdiv v10.2d, v11.2d, v12.2d | ||
| fdiv v10.2d, v11.2d, v12.2d | ||
| fdiv v0.2d, v0.2d, v1.2d |
There was a problem hiding this comment.
Curious why this sequence of fdiv needed to be changed. Does it matter to the test what regs are used with the fdiv?
| /* Return -1 if the signal happened before the critical block, 0 if it | ||
| * happened during the critical block, or 1 if it happened after the | ||
| * critical block or the timer was cancelled. Under DynamoRIO the | ||
| * signal will never appear to have happened during the critical block. |
There was a problem hiding this comment.
Maybe add to the comment that this is because we delay async signals to the end of the block
| @@ -1,6 +1,6 @@ | |||
| /* ********************************************************** | |||
There was a problem hiding this comment.
and hit the critical section about 400 times instead of about 1 time.
Optional: Curious if we tested this on x86?
The test is rewritten to repeatedly adjust the timer so that it hits the critical section more often. On AArch64 without DEBUG the new test ran in about 0.5 s instead of about 15 s and hit the critical section about 400 times instead of about 1 time.