-
Notifications
You must be signed in to change notification settings - Fork 546
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.
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
i#6745: Fix timestamp gap in delayed tracing #6746
base: master
Are you sure you want to change the base?
Conversation
Fixes a timestamp gap between the first and second timestamps in the trace caused when -trace_after_instrs is used. Fixes: #6745
Using manual runs verified that the diff between the first and second timestamps is much reduced:
|
@derekbruening I know we solved some timestamp gap related issues before. Do we have a unit test for it that I can add to? If not, do we want one now? |
Look at the PR's for the issues I linked in #6745: I see @bete0 added a unit test in PR #6165 or at least the description claims so |
I found two failures for the new test: on win64 and x86-32. Still debugging. |
x86-32 tool.drcachesim.opcode_mix failure is #6303 |
@derekbruening This is ready for review. PTAL. |
@@ -159,7 +158,7 @@ test_main(int argc, const char *argv[]) | |||
assert(status == scheduler_t::STATUS_OK); | |||
if (memref.marker.type == TRACE_TYPE_MARKER && | |||
memref.marker.marker_type == TRACE_MARKER_TYPE_TIMESTAMP) { | |||
// Looks at the gap between the current and the previous timestamps | |||
// Looks at the gap between the first and second timestamps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, should it check all of them (i.e., remove the break)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That equates to checking that there's no sleep between any two trace timestamps, which is not true.
OTOH there could be a sleep between the first two trace timestamps too. I feel this test is inherently brittle and may be working only because the -trace_after_instrs/-retrace_every_instrs/-trace_for_instrs settings are amenable.
Fixes a timestamp gap between the first and second timestamps in the trace caused when -trace_after_instrs is used.
Adds a unit test that reuses the existing window_test.cpp to reproduce the bug and fails without the fix.
Fixes: #6745