i#6104 multi-window: Refresh timestamps in a new window #6165
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
Updates the timestamp in the first buffer in a new window in multi-window traces. This fixes an issue where mult-window traces would have low activity at the beginning of a new window. Adds a unit test that fails without the fix in this change but passes with the change. Fixes #6143
|
Got the following error for Windows tests (https://github.com/DynamoRIO/dynamorio/actions/runs/5664479853/job/15347856021): Rerunning test. |
|
|
derekbruening
requested changes
Jul 26, 2023
- Add anonymous namespace - Put all system headers together - Remove temporary disgnostic logs - Add comments on what the window test does - Clean-up variable names - Add comment explaining the test approach - Rename first_timestamp to prior_timetsamp - Remove unused headers - Remove unused function param out_subdir
|
Best to get the test passing before requesting review: I see it failing on 4 different test jobs. |
derekbruening
requested changes
Jul 28, 2023
|
The new Also the test app results in 100+ windowed traces in Windows vs <20 in Linux. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
I pulled your branch onto a Windows machine and reproduced a popup which shows that popups are not disabled and that it is the debug libc: disable_popups() needs DEBUG set but that is not set: Looks like it's because you've disabled adding DEBUG: I don't see a configure.h being included in your test: so not sure why you have that line? Getting DEBUG there by removing that line should get rid of the popups and avoid the timeouts. |
Looks like that assert is from removing what DR treats as a syscall instr on WOW64 which is a call which has a store and the store is left dangling. |
derekbruening
added a commit
that referenced
this pull request
Aug 25, 2023
derekbruening
added a commit
that referenced
this pull request
Aug 25, 2023
derekbruening
added a commit
that referenced
this pull request
Sep 8, 2023
Adds a timestamp+cpuid pair at the end of normal buffers, to help separate trace output i/o time. Adds a timestamp+cpuid pair before and after each application syscall. It appears around the syscall marker. The trace buffer is no longer output pre-syscall (this addresses #3113) except for i-filtered traces or small-window traces where we need a frequent trace size check. Adds a timestamp+cpuid pair when a kernel transfer event occurs. The trace buffer is no longer output here. Fixes the existing behavior where the post-syscall-buffer's timestamp contains the pre-syscall time. Bumps the trace version for this change as the contents of post-syscall timestamps are now different. Augments the documentation around timestamps and adds a missing entry on syscall markers. Adds some invariant checks, and improve existing syscall marker checks. Further checks are more difficult as the post-syscall time is not present when there is no post-syscall event, such as with exit or sigreturn. Did some manual testing with the view tool. Applies several auxiliary changes required to get all modes to work with these additional timestamps: + Removes the timestamp+cpuid from the split-pipe header, as that timestamp is delayed by i/o and results in misleading out-of-order timestamps. + Prevents pipe-splitting before a cpuid, to avoid a confusing sequence. + Fixes a bug where a syscall marker is missing when the prior block wrote out its buffer (#6291). + Fixes #6245 by re-instating the footer for non-split window files and removing the template `.*` from PR #6165 which was hiding the warning (this one could possibly be split into its own PR). + Adjusts the PC discontinuity relaxation for online unfetched instrs (#4915) to check the size now that there's no timestamp. Issue: #4915 Fixes #6289 Fixes #3113 Fixes #6291 Fixes #6245
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.
Updates the timestamp in the first buffer of a new window in multi-window traces
by refreshing the timestamp when re-starting tracing. Previously, the timestamp in
the first buffer was coming from the end of a prior window causing low activity at the
beginning of a new window.
Adds a unit test that fails without the fix in this change but passes with the change.
Fixes an issue where multi-window tracing fails to write the last window raw file.
Fixes #6104
Fixes #6245