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 4 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
14 changes: 14 additions & 0 deletions clients/drcachesim/common/options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -333,3 +333,17 @@ 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.");

// XXX: replace function return address with function callstack
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the issue number (XXX i#3048:)

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

// XXX: default value should use the customized seperator when the relevant PR comes in
Copy link
Contributor

Choose a reason for hiding this comment

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

spelling: separator

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

#define DEFAULT_SEPARATOR " "
droption_t<std::string> op_record_function
(DROPTION_SCOPE_FRONTEND, "record_function", DROPTION_FLAG_ACCUMULATE,
std::string("malloc:0:1") + DEFAULT_SEPARATOR +
std::string("tc_malloc:1:1") + DEFAULT_SEPARATOR +
std::string("__libc_malloc:2:1"),
"Record invocation traces for the specified functions",
"Option value should fit this format <function_name>:<function_id>:<function_args_num>"
" The trace would contain information for function return address,"
" function argument values, and function return value. We only record pointer-sized"
" arguments and return value.");
1 change: 1 addition & 0 deletions clients/drcachesim/common/options.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,5 @@ 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;
#endif /* _OPTIONS_H_ */
26 changes: 26 additions & 0 deletions clients/drcachesim/common/trace_entry.h
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,32 @@ typedef enum {
*/
TRACE_MARKER_TYPE_CPU_ID,

/**
* The marker value contains the function id defined by 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.

s/user/the user/

Missing .

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,

/**
* The marker value contains the return address of the function call,
* whose id is specified by the closest previous FUNC_ID marker entry
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to get a link by using the full name and #: #TRACE_MARKER_TYPE_FUNC_ID.

Missing . again (let's keep these user-facing doxygen comments professional-looking)

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

* XXX: replace return address with callstack information in the future
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a comment to us, not the user, so put it in a separate regular (non-doxygen) comment (add the i# 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

*/
TRACE_MARKER_TYPE_FUNC_RETADDR,

/**
* The marker value contains one argument value of the function call,
* whose id is specified by the closest previous FUNC_ID marker entry.
* The number of such entries for one function invocation is expected to
* equal to the number of function arguments.
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, but the more important point is that it does equal the specified arguments in the -record_function option, right?

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_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 FUNC_ID marker entry
Copy link
Contributor

Choose a reason for hiding this comment

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

the return value, and turn FUNC_ID into a link

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_RETVAL,

// ...
// These values are reserved for future built-in marker types.
// ...
Expand Down
2 changes: 2 additions & 0 deletions clients/drcachesim/tests/basic_counts.templatex
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ Total counts:
#else
13 total transfer markers
#endif
.* total function markers
Copy link
Contributor

Choose a reason for hiding this comment

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

A slightly more specific test would be nice. Something like a simplified version of static_burst.cpp, where the app calls malloc, say, exactly 1000 times between dr_app_start() and dr_app_stop_and_cleanup(). Then we can run the basic_counts tool same way as here and check we've recorded them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Will add one in next commit

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

0 total other markers
Thread .* counts:
.* \(fetched\) instructions
Expand All @@ -45,4 +46,5 @@ Thread .* counts:
#else
13 transfer markers
#endif
.* function markers
0 other markers
129 changes: 129 additions & 0 deletions clients/drcachesim/tests/burst_malloc.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
/* **********************************************************
* 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 in the middle of the application. It then detaches.
Copy link
Contributor

Choose a reason for hiding this comment

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

Explain that it invokes malloc to test function tracing

*/

/* 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'"))
std::cerr << "failed to set env var!\n";

/* We use an outer loop to test re-attaching (i#2157). */
Copy link
Contributor

Choose a reason for hiding this comment

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

leftover comment from copy/paste?

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

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(1) < 0)
std::cerr << "error in computation\n";

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

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

/* FIXME i#2099: the weak symbol is not supported on Windows. */
#if defined(UNIX) && defined(TEST_APP_DR_CLIENT_MAIN)
# ifdef __cplusplus
extern "C" {
# endif

/* Test if the drmemtrace_client_main() in drmemtrace will be called. */
DR_EXPORT WEAK void
drmemtrace_client_main(client_id_t id, int argc, const char *argv[])
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to test the weak symbol in this test, you can remove this.

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

{
std::cerr << "wrong drmemtrace_client_main\n";
}

/* 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 */
2 changes: 2 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,7 @@ Total counts:
#else
13 total transfer markers
#endif
.* total function markers
0 total other markers
Thread .* counts:
.* \(fetched\) instructions
Expand All @@ -44,4 +45,5 @@ Thread .* counts:
#else
13 transfer markers
#endif
.* function markers
0 other markers
9 changes: 9 additions & 0 deletions clients/drcachesim/tests/offline-burst_malloc.templatex
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
pre-DR init
pre-DR start
pre-DR detach
DynamoRIO statistics:
*Peak threads under DynamoRIO control : *1
.*
all done
.*
.* 5000 function markers.*
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to have an analysis tool test that examines the function arg, return value, and retaddr entries so we know they're correct. OK in a separate CL.

Copy link
Contributor Author

@Louis-Ye Louis-Ye Jun 22, 2018

Choose a reason for hiding this comment

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

Since basic_counts.cpp is already modified in this CL for counting function markers, I will include the examines for different entries in this same CL. :)

Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,5 @@ Total counts:
4 total threads
*[0-9]* total scheduling markers
*[0-9]* total transfer markers
0 total function markers
0 total other markers
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ Total counts:
4 total threads
*[0-9]* total scheduling markers
*[0-9]* total transfer markers
0 total function markers
0 total other markers
.*
16 changes: 15 additions & 1 deletion clients/drcachesim/tools/basic_counts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ 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_other_markers(0), knob_verbose(verbose)
total_func_markers(0), total_other_markers(0), knob_verbose(verbose)
{
// Empty.
}
Expand All @@ -59,6 +59,15 @@ basic_counts_t::~basic_counts_t()
// Empty.
}

static bool
is_func_marker(trace_marker_type_t marker_type)
{
return marker_type == TRACE_MARKER_TYPE_FUNC_ID ||
marker_type == TRACE_MARKER_TYPE_FUNC_RETADDR ||
marker_type == TRACE_MARKER_TYPE_FUNC_ARG ||
marker_type == TRACE_MARKER_TYPE_FUNC_RETVAL;
}

bool
basic_counts_t::process_memref(const memref_t &memref)
{
Expand Down Expand Up @@ -86,6 +95,9 @@ basic_counts_t::process_memref(const memref_t &memref)
memref.marker.marker_type == TRACE_MARKER_TYPE_KERNEL_XFER) {
++thread_xfer_markers[memref.data.tid];
++total_xfer_markers;
} else if (is_func_marker(memref.marker.marker_type)) {
++thread_func_markers[memref.data.tid];
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, would total functions (just _RETVAL might be the way to count that: should cover tailcall too via drwrap) be more useful? Maybe not -- depends on what you want to count. Ignore this here. See prior comment about a function trace analysis tool.

++total_func_markers;
} else {
++thread_other_markers[memref.data.tid];
++total_other_markers;
Expand Down Expand Up @@ -116,6 +128,7 @@ 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_markers << " total function markers\n";
std::cerr << std::setw(12) << total_other_markers << " total other markers\n";

// Print the threads sorted by instrs.
Expand All @@ -134,6 +147,7 @@ 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_markers[tid] << " function markers\n";
std::cerr << std::setw(12) << thread_other_markers[tid] << " other markers\n";
}
return true;
Expand Down
2 changes: 2 additions & 0 deletions clients/drcachesim/tools/basic_counts.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ class basic_counts_t : public analysis_tool_t
int_least64_t total_stores;
int_least64_t total_sched_markers;
int_least64_t total_xfer_markers;
int_least64_t total_func_markers;
int_least64_t total_other_markers;
std::unordered_map<memref_tid_t, int_least64_t> thread_instrs;
std::unordered_map<memref_tid_t, int_least64_t> thread_instrs_nofetch;
Expand All @@ -63,6 +64,7 @@ class basic_counts_t : public analysis_tool_t
std::unordered_map<memref_tid_t, int_least64_t> thread_stores;
std::unordered_map<memref_tid_t, int_least64_t> thread_sched_markers;
std::unordered_map<memref_tid_t, int_least64_t> thread_xfer_markers;
std::unordered_map<memref_tid_t, int_least64_t> thread_func_markers;
std::unordered_map<memref_tid_t, int_least64_t> thread_other_markers;

unsigned int knob_verbose;
Expand Down
7 changes: 7 additions & 0 deletions clients/drcachesim/tracer/drmemtrace.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,13 @@ typedef enum {
DRMEMTRACE_ERROR_NOT_IMPLEMENTED, /**< Operation failed: not implemented. */
} drmemtrace_status_t;


/**
* Name of drmgr instrumentation pass priorities for app2app, analysis, insert,
Copy link
Contributor

Choose a reason for hiding this comment

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

Repeating prior comment (!): there is no app2app

Copy link
Contributor Author

@Louis-Ye Louis-Ye Jun 22, 2018

Choose a reason for hiding this comment

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

Oh, I saw in tracer.cpp that event_bb_app2app is registered as app2app event callback. Did I miss anything here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, you're right, my bad: I searched for "app2app" in the github diff view and it wasn't there (b/c it's on a line outside the context diff range) -- used for rep string expansion.

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

* and instru2instru.
*/
#define DRMGR_PRIORITY_NAME_MEMTRACE "memtrace"

DR_EXPORT
/**
* To support statically linking multiple clients on UNIX, dr_client_main() inside
Expand Down
Loading