-
Notifications
You must be signed in to change notification settings - Fork 576
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#6662 public traces: skip info.textproto and update doc #7406
base: master
Are you sure you want to change the base?
Conversation
Now that we added `info.textproto` files to public traces, we need to skip them when iterating over the content of a trace directory to avoid confusing the scheduler, as these files are meant for human-consumption only, not tools. We update the "Google workload traces" web page describing the intended use of `info.textproto`. We also clarify that memory operands in #DR_ISA_REGDEPS instructions can only be inferred from their subsequent read/write records. Issue #6662
the same v2p.textproto format. | ||
|
||
The info.textproto file provides users with additional, human-readable information about | ||
a trace. This file is not intended to be used with any DynamoRIO's tool. | ||
|
||
These traces are supported starting from DynamoRIO 11.3. |
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.
Technically this is not true anymore, the user would need to move info.textproto
out of the trace directory to have DR 11.3 work out of the box.
Maybe I should just remove the statement?
Should we spend some time on how to avoid this issue in the future?
It already happened for v2p.textproto
.
IMHO it's not ideal having to release a DR minor version every time a new file is added to a trace directory.
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.
Agreed, switch to an allowlist of supported compression extensions? Seems best to that now in this PR to avoid any more new releases?
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.
Agreed, switch to an allowlist of supported compression extensions?
Or can we move the aux files to their own separate directory inside the trace dir?
@@ -154,6 +154,10 @@ Further non-compatibility-affecting changes include: | |||
templates. Added similar options for the drmemtrace scheduler: #dynamorio::drmemtrace:: | |||
scheduler_tmpl_t::scheduler_options_t::kernel_syscall_trace_path, and #dynamorio:: | |||
drmemtrace::scheduler_tmpl_t::scheduler_options_t::kernel_syscall_reader. | |||
- Added info.textproto to the list of auxiliary files skipped when iterating over the | |||
content of a trace directory. This file is used to provide users with additional, | |||
human-readable information about #DR_ISA_REGDEPS traces. Currently, it contains |
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.
The peak live and phases info is not limited to REGDEPS so I would remove this part.
@@ -1527,6 +1527,13 @@ typedef struct _pt_data_buf_t pt_data_buf_t; | |||
*/ | |||
#define DRMEMTRACE_V2P_FILENAME "v2p.textproto" | |||
|
|||
/** | |||
* The name of the file in that contains metadata information about a #DR_ISA_REGDEPS |
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.
Ditto: remove REGDEPS.
@@ -1527,6 +1527,13 @@ typedef struct _pt_data_buf_t pt_data_buf_t; | |||
*/ | |||
#define DRMEMTRACE_V2P_FILENAME "v2p.textproto" | |||
|
|||
/** | |||
* The name of the file in that contains metadata information about a #DR_ISA_REGDEPS |
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.
s/in//
with 2 MB pages. Users can generate different mappings (e.g., smaller page size) | ||
by modifying this file, or create their own mapping following the same | ||
v2p.textproto format. | ||
Every trace has a v2p.textproto and an info.textproto file associated to it. |
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.
s/to/with/
the same v2p.textproto format. | ||
|
||
The info.textproto file provides users with additional, human-readable information about | ||
a trace. This file is not intended to be used with any DynamoRIO's tool. |
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.
s/'s//
fname == DRMEMTRACE_ENCODING_FILENAME || fname == DRMEMTRACE_V2P_FILENAME) | ||
fname == DRMEMTRACE_ENCODING_FILENAME || | ||
fname == DRMEMTRACE_V2P_FILENAME || | ||
fname == DRMEMTRACE_TRACE_INFO_FILENAME) |
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.
As noted, let's switch to an allowlist of known compression suffixes instead? Or look for "drmemtrace" in the file name?
@@ -1812,7 +1814,8 @@ scheduler_impl_tmpl_t<RecordType, ReaderType>::open_readers( | |||
// Skip the auxiliary files. | |||
if (fname == DRMEMTRACE_MODULE_LIST_FILENAME || | |||
fname == DRMEMTRACE_FUNCTION_LIST_FILENAME || | |||
fname == DRMEMTRACE_ENCODING_FILENAME || fname == DRMEMTRACE_V2P_FILENAME) | |||
fname == DRMEMTRACE_ENCODING_FILENAME || fname == DRMEMTRACE_V2P_FILENAME || | |||
fname == DRMEMTRACE_TRACE_INFO_FILENAME) |
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.
Ditto.
Now that we added
info.textproto
files to public traces, we need to skipthem when iterating over the content of a trace directory to avoid confusing
the scheduler, as these files are meant for human-consumption only, not tools.
We update the "Google workload traces" web page describing the intended
use of
info.textproto
. We also clarify that memory operands in#DR_ISA_REGDEPS instructions can only be inferred from their subsequent
read/write records.
Issue #6662