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#3048 drcachesim record funcs: infrastructure #3057

Merged
merged 25 commits into from
Jun 27, 2018
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1e28578
i#3048 memtrace-func: add function traces in memtrace tracer
Louis-Ye Jun 19, 2018
f99373e
i#3048 memtrace-func: add option to enable func trace
Louis-Ye Jun 19, 2018
093cb57
i#3048 memtrace-func: check return value of drmgr_register
Louis-Ye Jun 19, 2018
3ba97a3
i#3048 memtrace-func: separate func_trace module, add test, use optio…
Louis-Ye Jun 20, 2018
d1713b9
Merge branch 'master' into i3048-drcachesim-func-traces
Louis-Ye Jun 21, 2018
844b9a3
Merge branch 'master' into i3048-drcachesim-func-traces
Louis-Ye Jun 21, 2018
bb1ead5
i#3048 memtrace-func: use drvector_t to replace vector
Louis-Ye Jun 22, 2018
32a8f98
i#3048 memtrace-func: fix style by add newline at end of file
Louis-Ye Jun 22, 2018
647ecec
Merge branch 'master' into i3048-drcachesim-func-traces
Louis-Ye Jun 22, 2018
acba3bd
i#3048 memtrace-func: add -record_heap, update comments, count markers
Louis-Ye Jun 22, 2018
871c03c
i#3048 memtrace-func: add separator when both -record_function and -r…
Louis-Ye Jun 22, 2018
6ec4a13
i#3048 memtrace-func: Use & as item separator
Louis-Ye Jun 23, 2018
23e725c
i#3048 memtrace-func: notify instead of crash on duplicate id
Louis-Ye Jun 24, 2018
f1ba45b
i#3048 memtrace-func: update comments
Louis-Ye Jun 24, 2018
0c170a9
i#3048 memtrace-func: use BUFFER_SIZE_ELEMENTS
Louis-Ye Jun 25, 2018
fafa638
Merge branch 'master' into i3048-drcachesim-func-traces
Louis-Ye Jun 26, 2018
90fc899
i#3048 memtrace-func: save input func str, update comments and docs
Louis-Ye Jun 26, 2018
341f812
i#3048 memtrace-func: get rid of DR_EXPORT
Louis-Ye Jun 26, 2018
468d837
i#3048 memtrace-func: update comments
Louis-Ye Jun 26, 2018
1943e10
i#3048 memtrace-func: update comments
Louis-Ye Jun 26, 2018
d635d36
i#3048 memtrace-func: add -V to runsuite_wrapper.pl
Louis-Ye Jun 26, 2018
d358bf2
i#3048 memtrace-func: add -VV
Louis-Ye Jun 26, 2018
8d6c0a2
i#3048 memtrace-func: add explicit cast for windows build
Louis-Ye Jun 26, 2018
4f45cdf
i#3048 memtrace-func: change -VV back to -V
Louis-Ye Jun 26, 2018
0f64c4b
Merge branch 'master' into i3048-drcachesim-func-traces
derekbruening Jun 27, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions clients/drcachesim/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,13 @@ macro(add_drmemtrace name type)
tracer/instru_offline.cpp
tracer/instru_online.cpp
tracer/physaddr.cpp
tracer/func_trace.cpp
${client_and_sim_srcs}
)
configure_DynamoRIO_client(${name})
use_DynamoRIO_extension(${name} drmgr${ext_sfx})
use_DynamoRIO_extension(${name} drsyms${ext_sfx})
use_DynamoRIO_extension(${name} drwrap${ext_sfx})
use_DynamoRIO_extension(${name} drreg${ext_sfx})
use_DynamoRIO_extension(${name} drutil${ext_sfx})
use_DynamoRIO_extension(${name} drx${ext_sfx})
Expand Down Expand Up @@ -430,6 +433,11 @@ if (BUILD_TESTS)
use_DynamoRIO_extension(tool.drcacheoff.burst_replaceall drcontainers)
use_DynamoRIO_drmemtrace_tracer(tool.drcacheoff.burst_replaceall)

add_executable(tool.drcacheoff.burst_malloc tests/burst_malloc.cpp)
configure_DynamoRIO_static(tool.drcacheoff.burst_malloc)
use_DynamoRIO_static_client(tool.drcacheoff.burst_malloc drmemtrace_static)
add_win32_flags(tool.drcacheoff.burst_malloc)

add_executable(tool.drcacheoff.burst_threads tests/burst_threads.cpp)
configure_DynamoRIO_static(tool.drcacheoff.burst_threads)
use_DynamoRIO_static_client(tool.drcacheoff.burst_threads drmemtrace_static)
Expand Down
36 changes: 36 additions & 0 deletions clients/drcachesim/common/options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -333,3 +333,39 @@ droption_t<bool> op_reuse_verify_skip
"Verifies every skip list-calculated reuse distance with a full list walk. "
"This incurs significant additional overhead. This option is only available "
"in debug builds.");

#define OP_RECORD_FUNC_ITEM_SEP "&"
// XXX i#3048: replace function return address with function callstack
// XXX i#3048: add a section to drcachesim.dox.in on function tracing
droption_t<std::string> op_record_function
(DROPTION_SCOPE_ALL, "record_function", DROPTION_FLAG_ACCUMULATE,
OP_RECORD_FUNC_ITEM_SEP, "",
"Record invocations trace for the specified function(s).",
"Record invocations trace for the specified function(s) in the option"
" value. Default value is empty. The value should fit this format:"
" function_name|function_id|func_args_num"
" (example: -record_function \"memset|10|3\"). The trace would contain"
" information for function return address, function argument value(s),"
" and function return value. We only record pointer-sized arguments and"
" return value. The trace is labeled with the function_id via an ID entry"
" prior to each set of value entries."
" Note that the provided function id should be unique, and not collide with"
" existing heap functions (see -record_heap_value ) if record_heap"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: inconsistent option reference: -record_heap
nit: space before )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

" option is enabled.");
droption_t<bool> op_record_heap
(DROPTION_SCOPE_ALL, "record_heap", false,
"Enable recording trace for the defined heap functions.",
"It is a convenient option to enable recording trace for the defined heap"
Copy link
Contributor

Choose a reason for hiding this comment

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

s/convenient/convenience/
grammar: s/trace/a trace/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

" functions in -record_heap_value. Specify this option is equivalent to"
Copy link
Contributor

Choose a reason for hiding this comment

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

grammar: s/Specify/Specifying/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

" -record_function [heap_functions], where [heap_functions] is"
" the value in -record_heap_value.");
droption_t<std::string> op_record_heap_value
(DROPTION_SCOPE_ALL, "record_heap_value", DROPTION_FLAG_ACCUMULATE,
OP_RECORD_FUNC_ITEM_SEP,
"malloc|0|1" OP_RECORD_FUNC_ITEM_SEP "free|1|1" OP_RECORD_FUNC_ITEM_SEP
"tc_malloc|2|1" OP_RECORD_FUNC_ITEM_SEP "tc_free|3|1" OP_RECORD_FUNC_ITEM_SEP
"__libc_malloc|4|1" OP_RECORD_FUNC_ITEM_SEP "__libc_free|5|1",
" The actual value of the defined heap functions.",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would instead say "Functions recorded by -record_heap". No leading space. (To the reader it is confusing as to what "the defined heap functions" is referring to. I would update the start of the long description too.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

" The actual value of the defined heap functions. Value should fit the same"
" format required by -record_function. These functions will not"
Copy link
Contributor

Choose a reason for hiding this comment

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

The separator is not described under -record_function, though, so that makes it a little confusing. Probably best to explain the separator under -record_function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

" be traced unless -record_heap is specified.");
3 changes: 3 additions & 0 deletions clients/drcachesim/common/options.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,7 @@ extern droption_t<unsigned int> op_reuse_distance_threshold;
extern droption_t<bool> op_reuse_distance_histogram;
extern droption_t<unsigned int> op_reuse_skip_dist;
extern droption_t<bool> op_reuse_verify_skip;
extern droption_t<std::string> op_record_function;
extern droption_t<bool> op_record_heap;
extern droption_t<std::string> op_record_heap_value;
#endif /* _OPTIONS_H_ */
29 changes: 29 additions & 0 deletions clients/drcachesim/common/trace_entry.h
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,35 @@ typedef enum {
*/
TRACE_MARKER_TYPE_CPU_ID,

/**
* The marker value contains function id defined by the user in droption.
Copy link
Contributor

Choose a reason for hiding this comment

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

grammar: s/function/the function/

The user doesn't know what "in droption" means: s/droption/the -record_function option/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

*/
TRACE_MARKER_TYPE_FUNC_ID,

// XXX i#3048: replace return address with callstack information.
/**
* The marker value contains the return address of the function call,
* whose id is specified by the closest previous
Copy link
Contributor

Choose a reason for hiding this comment

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

My prior review comments here were not acted on: if you disagree with a review request, that's certainly your prerogative, but please reply either way to say whether you're acting on the request or why you're not -- please don't skip over it with no comment because then the reviewer wonders whether some other requests were silently skipped over...I shouldn't have to go double-check all my prior comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Skipping those comments is definitely NOT deliberate... It is my bad to skip them. Because sometimes I made several commits, and a non-relevant code change might touch the lines that are covered by a comment, which will then turn this comment into hidden, which cause me to skip it. I should have been aware of that a hidden commit is not necessary addressed.
In later commit, I will manually reply all comments (including all the previous one) to ensure that they are addressed. Sorry for the inconvenience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* marker entry.
*/
TRACE_MARKER_TYPE_FUNC_RETADDR,

/**
* The marker value contains one argument value of the function call,
* whose id is specified by the closest previous #TRACE_MARKER_TYPE_FUNC_ID
* marker entry. The number of such entries for one function invocation is
* expected to be equal to the specified arugment in -record_function or
Copy link
Contributor

Choose a reason for hiding this comment

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

My prior review comment was trying to say, why say "expected to be"? That makes it sound like either it's not under our control or it varies somehow...just say "is equal".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

spelling (please write these user-facing docs carefully, just like you would with code): argument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

* pre-defined functions in -record_heap (if enabled).
*/
TRACE_MARKER_TYPE_FUNC_ARG,

/**
* The marker value contains return value of the function call,
Copy link
Contributor

Choose a reason for hiding this comment

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

Another skipped prior review 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.

Done

* whose id is specified by the closest previous #TRACE_MARKER_TYPE_FUNC_ID
* marker entry
*/
TRACE_MARKER_TYPE_FUNC_RETVAL,

// ...
// These values are reserved for future built-in marker types.
// ...
Expand Down
8 changes: 8 additions & 0 deletions clients/drcachesim/tests/basic_counts.templatex
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ Total counts:
#else
13 total transfer markers
#endif
.* total function id markers
.* total function return address markers
.* total function argument markers
.* total function return value markers
0 total other markers
Thread .* counts:
.* \(fetched\) instructions
Expand All @@ -45,4 +49,8 @@ Thread .* counts:
#else
13 transfer markers
#endif
.* function id markers
.* function return address markers
.* function argument markers
.* function return value markers
0 other markers
126 changes: 126 additions & 0 deletions clients/drcachesim/tests/burst_malloc.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/* **********************************************************
* Copyright (c) 2016-2017 Google, Inc. All rights reserved.
* **********************************************************/

/*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
*
* * Redistributions of source code must retain the above copyright notice,
* this list of conditions and the following disclaimer.
*
* * Redistributions in binary form must reproduce the above copyright notice,
* this list of conditions and the following disclaimer in the documentation
* and/or other materials provided with the distribution.
*
* * Neither the name of Google, Inc. nor the names of its contributors may be
* used to endorse or promote products derived from this software without
* specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL GOOGLE, INC. OR CONTRIBUTORS BE LIABLE
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
* SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
* CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
* DAMAGE.
*/

/* This application links in drmemtrace_static and acquires a trace during
* a "burst" of execution and memory allocations (malloc() and free())
* in the middle of the application. It then detaches. Later it re-attaches
* and detaches again for several times.
*/

/* Like burst_static we deliberately do not include configure.h here */

#include "dr_api.h"
#include <assert.h>
#include <iostream>
#include <math.h>
#include <stdlib.h>

bool
my_setenv(const char *var, const char *value)
{
#ifdef UNIX
return setenv(var, value, 1/*override*/) == 0;
#else
return SetEnvironmentVariable(var, value) == TRUE;
#endif
}

static int
do_some_work(int arg)
{
static const int iters = 1000;
static double *vals[iters];

double val = (double)arg;
for (int i = 0; i < iters; ++i) {
vals[i] = (double *)malloc(sizeof(double));
*vals[i] = sin(val);
val += *vals[i];
}
for (int i = 0; i < iters; i++) {
val += *vals[i];
}
for (int i = 0; i < iters; i++) {
free(vals[i]);
}
return (val > 0);
}

int
main(int argc, const char *argv[])
{
/* We also test -rstats_to_stderr */
if (!my_setenv("DYNAMORIO_OPTIONS", "-stderr_mask 0xc -rstats_to_stderr"
" -client_lib ';;-offline -record_heap"
" -record_function \"malloc|0|1\"'"))
std::cerr << "failed to set env var!\n";

for (int i = 0; i < 3; i++) {
std::cerr << "pre-DR init\n";
dr_app_setup();
assert(!dr_app_running_under_dynamorio());

std::cerr << "pre-DR start\n";
if (do_some_work(i * 1) < 0)
std::cerr << "error in computation\n";

dr_app_start();
if (do_some_work(i * 2) < 0)
std::cerr << "error in computation\n";
std::cerr << "pre-DR detach\n";
dr_app_stop_and_cleanup();

if (do_some_work(i * 3) < 0)
std::cerr << "error in computation\n";
std::cerr << "all done\n";
}

return 0;
}

#if defined(UNIX) && defined(TEST_APP_DR_CLIENT_MAIN)
# ifdef __cplusplus
extern "C" {
# endif

/* This dr_client_main should be called instead of the one in tracer.cpp */
DR_EXPORT void
dr_client_main(client_id_t id, int argc, const char *argv[])
{
std::cerr << "app dr_client_main\n";
drmemtrace_client_main(id, argc, argv);
}

# ifdef __cplusplus
}
# endif
#endif /* UNIX && TEST_APP_DR_CLIENT_MAIN */
8 changes: 8 additions & 0 deletions clients/drcachesim/tests/offline-basic_counts.templatex
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ Total counts:
#else
13 total transfer markers
#endif
.* total function id markers
.* total function return address markers
.* total function argument markers
.* total function return value markers
0 total other markers
Thread .* counts:
.* \(fetched\) instructions
Expand All @@ -44,4 +48,8 @@ Thread .* counts:
#else
13 transfer markers
#endif
.* function id markers
.* function return address markers
.* function argument markers
.* function return value markers
0 other markers
14 changes: 14 additions & 0 deletions clients/drcachesim/tests/offline-burst_malloc.templatex
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
pre-DR init
Warning: duplicated function id .*
.*
pre-DR start
pre-DR detach
DynamoRIO statistics:
*Peak threads under DynamoRIO control : *1
.*
all done
.*
.* 4000 function id markers.*
.* 2000 function return address markers.*
.* 2000 function argument markers.*
.* 2000 function return value markers.*
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,8 @@ Total counts:
4 total threads
*[0-9]* total scheduling markers
*[0-9]* total transfer markers
0 total function id markers
0 total function return address markers
0 total function argument markers
0 total function return value markers
0 total other markers
4 changes: 4 additions & 0 deletions clients/drcachesim/tests/offline-burst_threadfilter.templatex
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,9 @@ Total counts:
4 total threads
*[0-9]* total scheduling markers
*[0-9]* total transfer markers
0 total function id markers
0 total function return address markers
0 total function argument markers
0 total function return value markers
0 total other markers
.*
42 changes: 40 additions & 2 deletions clients/drcachesim/tools/basic_counts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ basic_counts_tool_create(unsigned int verbose)
basic_counts_t::basic_counts_t(unsigned int verbose) :
total_threads(0), total_instrs(0), total_instrs_nofetch(0), total_prefetches(0),
total_loads(0), total_stores(0), total_sched_markers(0), total_xfer_markers(0),
total_func_id_markers(0), total_func_retaddr_markers(0),
total_func_arg_markers(0), total_func_retval_markers(0),
total_other_markers(0), knob_verbose(verbose)
{
// Empty.
Expand Down Expand Up @@ -87,8 +89,28 @@ basic_counts_t::process_memref(const memref_t &memref)
++thread_xfer_markers[memref.data.tid];
++total_xfer_markers;
} else {
++thread_other_markers[memref.data.tid];
++total_other_markers;
switch(memref.marker.marker_type) {
case TRACE_MARKER_TYPE_FUNC_ID:
++total_func_id_markers;
++thread_func_id_markers[memref.data.tid];
break;
case TRACE_MARKER_TYPE_FUNC_RETADDR:
++total_func_retaddr_markers;
++thread_func_retaddr_markers[memref.data.tid];
break;
case TRACE_MARKER_TYPE_FUNC_ARG:
++total_func_arg_markers;
++thread_func_arg_markers[memref.data.tid];
break;
case TRACE_MARKER_TYPE_FUNC_RETVAL:
++total_func_retval_markers;
++thread_func_retval_markers[memref.data.tid];
break;
default:
++thread_other_markers[memref.data.tid];
++total_other_markers;
break;
}
}
} else if (memref.exit.type == TRACE_TYPE_THREAD_EXIT) {
++total_threads;
Expand Down Expand Up @@ -116,6 +138,14 @@ basic_counts_t::print_results() {
std::cerr << std::setw(12) << total_threads << " total threads\n";
std::cerr << std::setw(12) << total_sched_markers << " total scheduling markers\n";
std::cerr << std::setw(12) << total_xfer_markers << " total transfer markers\n";
std::cerr << std::setw(12) << total_func_id_markers <<
" total function id markers\n";
std::cerr << std::setw(12) << total_func_retaddr_markers <<
" total function return address markers\n";
std::cerr << std::setw(12) << total_func_arg_markers <<
" total function argument markers\n";
std::cerr << std::setw(12) << total_func_retval_markers <<
" total function return value markers\n";
std::cerr << std::setw(12) << total_other_markers << " total other markers\n";

// Print the threads sorted by instrs.
Expand All @@ -134,6 +164,14 @@ basic_counts_t::print_results() {
std::cerr << std::setw(12) << thread_sched_markers[tid] <<
" scheduling markers\n";
std::cerr << std::setw(12) << thread_xfer_markers[tid] << " transfer markers\n";
std::cerr << std::setw(12) << thread_func_id_markers[tid] <<
" function id markers\n";
std::cerr << std::setw(12) << thread_func_retaddr_markers[tid] <<
" function return address markers\n";
std::cerr << std::setw(12) << thread_func_arg_markers[tid] <<
" function argument markers\n";
std::cerr << std::setw(12) << thread_func_retval_markers[tid] <<
" function return value markers\n";
std::cerr << std::setw(12) << thread_other_markers[tid] << " other markers\n";
}
return true;
Expand Down
Loading