Skip to content

Conversation

@GandholiSarat
Copy link
Member

Instruction records with type TRACE_TYPE_INSTR_NO_FETCH were previously displayed as normal ifetch entries in the drmemtrace view tool when decoding was enabled, which was misleading for REP string expansion.

This change updates the view tool output to label such records as non-fetched, without altering trace generation or expansion logic.

Xref #4915.

Instruction records with type TRACE_TYPE_INSTR_NO_FETCH were previously
displayed as normal ifetch entries in the drmemtrace view tool when
decoding was enabled, which was misleading for REP string expansion.

This change updates the view tool output to label such records as
non-fetched, without altering trace generation or expansion logic.

Xref i#4915.
Add an x86-only subtest in view_test.cpp to verify that
TRACE_TYPE_INSTR_NO_FETCH records are labeled as
"non-fetched" in the drmemtrace view tool output.

Xref #4915.
@GandholiSarat
Copy link
Member Author

I’ve added an x86-only test in view_test.cpp to verify that TRACE_TYPE_INSTR_NO_FETCH records are labeled as "non-fetched" in the view tool output.

This is my first time adding a test in this part of the drmemtrace suite, so please let me know if you’d prefer any adjustments to structure or style.

add_encodings_to_memrefs(ilist, memref_setup, BASE_ADDR);

view_test_t view(drcontext, *ilist);
std::string res = run_test_helper(view, memrefs);
Copy link
Contributor

Choose a reason for hiding this comment

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

No change requested: just talking to myself here:

Didn't realize this run_test_helper was here: unused by other tests which all use trace_entry_t to go through the scheduler. But this seems ok as this test is not checking anything that involves the scheduler.

A real trace would have data records between the instr records: but not a big deal.

Maybe worth ensuring the 1st one is a real ifetch: but more of what we'd put in an end to end test that included the rep expansion.

So seems reasonable as is.

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