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#7113 decode cache: Refactor view_t to use the new lib #7219

Merged
merged 11 commits into from
Jan 30, 2025

Conversation

abhinav92003
Copy link
Contributor

@abhinav92003 abhinav92003 commented Jan 27, 2025

Refactors the view_t tool to use the new decode_cache_t instead of duplicating the instr decoding logic.

Modifies decode_info_base_t::set_decode_info_derived to allow it to return the error string. This is useful for cases where there was an error during its operation, which is especially possible when used with a decode_cache_t with include_decoded_instr_ = false in which case the decode_info_t also performs the decoding on its own.

Adds new tests to decode_cache_test to verify some related cases. Not adding a new release note for the API signature change as there was no intervening release since it was initially added.

Also fixes the view_t logic to get filetype, which should be obtained from the memtrace_stream_t object instead of TRACE_MARKER_TYPE_FILETYPE to handle cases where the filetype marker is not seen by the tool because of analyzer options like -skip_instrs that skip over a certain initial part of the trace.

Issue: #7113, #7224

Refactors the view_t to use the new decode_cache_t instead of duplicating the logic.

Modifies decode_info_base_t::set_decode_info_derived to allow it to return the error
string. This is useful for cases where the decode_info_t object encountered some
error during its operation, which is especially possible when used with a
decode_cache_t with include_decoded_instr_=false in which case the decode_info_t
also performs the decoding on its own. Adds new tests to decode_cache_test to
verify some related cases.

Issue: #7113
Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

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

Not sure about this sudden re-interpretation of OFFLINE_FILE_TYPE_DEFAULT as a sentinel that cannot happen in a real trace.

clients/drcachesim/tests/decode_cache_test.cpp Outdated Show resolved Hide resolved
clients/drcachesim/tests/decode_cache_test.cpp Outdated Show resolved Hide resolved
clients/drcachesim/tests/decode_cache_test.cpp Outdated Show resolved Hide resolved
clients/drcachesim/tests/view_test.cpp Outdated Show resolved Hide resolved
clients/drcachesim/tools/common/decode_cache.h Outdated Show resolved Hide resolved
clients/drcachesim/tools/view.cpp Show resolved Hide resolved
clients/drcachesim/tools/view.cpp Outdated Show resolved Hide resolved
clients/drcachesim/tools/view.cpp Outdated Show resolved Hide resolved
clients/drcachesim/tools/view.cpp Show resolved Hide resolved
clients/drcachesim/tools/view.cpp Show resolved Hide resolved
@abhinav92003 abhinav92003 merged commit 8950487 into master Jan 30, 2025
17 checks passed
@abhinav92003 abhinav92003 deleted the i7113-view-decode-cache branch January 30, 2025 04:54
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