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.

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#5538 memtrace seek, part 10: Top-level headers #5739

Merged
merged 6 commits into from
Nov 17, 2022

Conversation

derekbruening
Copy link
Contributor

Adds cached values of the 5 top-level headers to the metrace_stream_t interface and implements this for the readers.

Adds checks of these values to invariant_checker.

Adds a test with skipped instructions using the skip_unit_tests checked-in trace.

Reverses the order of the initial 2 markers and the tid,pid pair sent to the reader to avoid a 0 tid in tools. I am surprised this hasn't caused more problems and I thought it was already this fixed way.

Issue: #5538

Adds cached values of the 5 top-level headers to the metrace_stream_t
interface and implements this for the readers.

Adds checks of these values to invariant_checker.

Adds a test with skipped instructions using the skip_unit_tests
checked-in trace.

Reverses the order of the initial 2 markers and the tid,pid pair sent
to the reader to avoid a 0 tid in tools.  I am surprised this hasn't
caused more problems and I thought it was already this fixed way.

Issue: #5538
…online bundles; Disable ordinal checks for serial multithread mode
@derekbruening
Copy link
Contributor Author

x32 failure is api.startstop assert about sysenter #4604

@@ -221,6 +221,7 @@ template <typename T> class file_reader_t : public reader_t {
return false;
}
// Read the meta entries until we hit the pid.
std::queue<trace_entry_t> marker_queue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this intermediate marker_queue accomplish? That the markers are stored after the tid and pid? Add comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the file has marker,marker,tid,pid but the reader has to fill in the tid+pid fields of memref_t so it is best for it to see the tid+pid first (o/w tools doing per-thread data in serial mode have problems). Added a comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could consider reordering the original order of the fields in raw2trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would avoid the reordering multiple pieces of code are having to do right now. But it makes it hard to change things with a version bump b/c the version is not in the first record so we'd be locked into that format? There are tradeoffs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to skip trace format change for now. Looks like it needs more thought.

clients/drcachesim/reader/reader.cpp Show resolved Hide resolved
clients/drcachesim/reader/file_reader.h Outdated Show resolved Hide resolved
clients/drcachesim/reader/reader.h Outdated Show resolved Hide resolved
clients/drcachesim/tools/invariant_checker.cpp Outdated Show resolved Hide resolved
Adds a reader subclass to raw2trace for computing the memref_t record
count for each chunk.  A new marker is inserted in the chunk header
which the zipfile skip code uses to obtain the correct ref count when
skipping over chunks.

Adds a count suppression feature where the view tool prints 0 for the
record count for the synthetic timestamp+cpu added after a seek.

Updates the seek test with a new trace and new expected output.

Issue: #5538
…); update skip test output for prior PR with 0 for inserted time,cpu
@derekbruening
Copy link
Contributor Author

run arm tests

@derekbruening
Copy link
Contributor Author

Hitting the same attach timeouts it seems on Jenkins: #5740

@abhinav92003
Copy link
Contributor

Hitting the same attach timeouts it seems on Jenkins: #5740

We probably should add it to the ignore-list?

@derekbruening
Copy link
Contributor Author

Re-run is still hitting the attach ones: someone will have to put effort into fixing or adding to ignore list. Also hit the inv checker on the re-run (not on prior runs): #5724.

@derekbruening derekbruening merged commit cd7bd0d into master Nov 17, 2022
@derekbruening derekbruening deleted the i5538-global-headers branch November 17, 2022 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants