i#3048 drcachesim record funcs: infrastructure #3057

Louis-Ye · 2018-06-19T01:23:39Z

Add function traces as extended marker type in memtrace (drcachesim) client.
For each invocation of the specified functions in runtime options, the trace contains its return address, argument(s) value(s), and return value.
Currently we support recording pointer-size arguments and return value.

#3048

Add function traces as extended marker type. This PR contains only hard-coded malloc for tracing, but it is easily extended to trace other functions as well. For each invocation of the pre-defined function, the trace contains its return address, each argument values, and return value Fixes DynamoRIO#3048

Add option to enable func trace. Make drwrap init at beginning and exit at the end. Fixes DynamoRIO#3048

Fixes DynamoRIO#3048

s-kanev · 2018-06-19T15:31:22Z

clients/drcachesim/tracer/tracer.cpp

+    const int num_args;
+} func_metadata;
+
+#define MALLOC_RETADDR TRACE_MARKER_TYPE_FUNC_MALLOC_RETADDR


Not sure you gain much from these, other than saving a few keystrokes. I'd remove them.

s-kanev · 2018-06-19T15:35:58Z

clients/drcachesim/tracer/tracer.cpp

+#define MALLOC_ARG     TRACE_MARKER_TYPE_FUNC_MALLOC_ARG
+#define MALLOC_RETVAL  TRACE_MARKER_TYPE_FUNC_MALLOC_RETVAL
+
+static const func_metadata instru_funcs[] = {


We'll obviously need to expand the list (especially with all the operator new variants), but I think it's fine to leave that for later.

I'd remove do_malloc and malloc_impl because they sound like implementation details of particular allocators and it's not guaranteed that the arguments / return values are what we expect them to be.

I ported the do_malloc and malloc_impl from drmemory, and they seem to have the same signature as malloc. Do you think it is safe enough to keep them here?

No idea about malloc_impl (I wouldn't worry too much about windows outside of breaking DR there).

do_malloc() is from an old version of tcmalloc and doesn't exist any more even upstream (https://github.com/gperftools/gperftools/search?q=do_malloc&unscoped_q=do_malloc). Even if it did, I don't think we should've kept it. It was an internal helper routine called by malloc(), so if we trace both, we'd record the same call twice (unless we taught the tracer we're in the same context, but that sounds like unnecessary complexity).

I lied, it still exists upstream [1]. Still, we should ignore it so we don't double record.

[1] https://github.com/gperftools/gperftools/blob/f47a52ce85c3d8d559aaae7b7a426c359fbca225/src/tcmalloc.cc#L1348

OK, will remove those. I guess it is also good to keep it simple as much as possible in this PR.

s-kanev · 2018-06-19T15:38:27Z

clients/drcachesim/tracer/tracer.cpp

+        return; /* This thread was filtered out. */
+
+    func_metadata *e = (func_metadata *)user_data;
+    void * retval = drwrap_get_retval(wrapcxt);


nit: void *retval

s-kanev · 2018-06-19T15:38:41Z

clients/drcachesim/tracer/tracer.cpp

+                              (uintptr_t)retaddr);
+
+    for (int i = 0; i < e->num_args; i++) {
+        void * arg_i = drwrap_get_arg(wrapcxt, i);


nit: void *arg_i
https://github.com/DynamoRIO/dynamorio/wiki/Code-Style#types

s-kanev · 2018-06-19T15:47:35Z

clients/drcachesim/tracer/tracer.cpp

@@ -1006,6 +1016,7 @@ event_app_instruction(void *drcontext, void *tag, instrlist_t *bb,
        !op_L0_filter.get_value() &&
        // The delay instr buffer is not full.
        ud->num_delay_instrs < MAX_NUM_DELAY_INSTRS) {
+


nit: remove

s-kanev · 2018-06-19T15:57:44Z

clients/drcachesim/tests/basic_counts.templatex

@@ -32,6 +32,7 @@ Total counts:
 #else
          13 total transfer markers
 #endif
+     .* total function markers


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.

Sure! Will add one in next commit

derekbruening · 2018-06-19T03:21:56Z

clients/drcachesim/tools/basic_counts.cpp

@@ -59,6 +59,13 @@ basic_counts_t::~basic_counts_t()
    // Empty.
 }

+static bool is_func_marker(trace_marker_type_t marker_type)


style violation: newline after return type

derekbruening · 2018-06-19T15:54:41Z

clients/drcachesim/common/options.cpp

+
+droption_t<bool> op_enable_func_trace
+(DROPTION_SCOPE_FRONTEND, "enable_func_trace", true,
+ "enable traces for function invocations",


Follow the conventions of the other options: the short description should be capitalized with ending punctuation.

derekbruening · 2018-06-19T15:55:42Z

clients/drcachesim/common/options.cpp

+droption_t<bool> op_enable_func_trace
+(DROPTION_SCOPE_FRONTEND, "enable_func_trace", true,
+ "enable traces for function invocations",
+ "The function trace would contain information of arguments, return address, and "


See how this is placed into the html docs: the short is not duplicated, so please include an option description at the start of the long that does not assume the short.

derekbruening · 2018-06-19T15:56:05Z

clients/drcachesim/common/options.cpp

+(DROPTION_SCOPE_FRONTEND, "enable_func_trace", true,
+ "enable traces for function invocations",
+ "The function trace would contain information of arguments, return address, and "
+ "return value for invocations of the pre-defined functions");


derekbruening · 2018-06-19T16:09:29Z

clients/drcachesim/common/options.cpp

+(DROPTION_SCOPE_FRONTEND, "enable_func_trace", true,
+ "enable traces for function invocations",
+ "The function trace would contain information of arguments, return address, and "
+ "return value for invocations of the pre-defined functions");


This needs to explain which functions: how would a user have any idea what "the pre-defined functions" are?

derekbruening · 2018-06-19T17:01:00Z

clients/drcachesim/tracer/tracer.cpp

@@ -1006,6 +1016,7 @@ event_app_instruction(void *drcontext, void *tag, instrlist_t *bb,
        !op_L0_filter.get_value() &&
        // The delay instr buffer is not full.
        ud->num_delay_instrs < MAX_NUM_DELAY_INSTRS) {
+


nit: remove

derekbruening · 2018-06-19T17:04:34Z

clients/drcachesim/tracer/tracer.cpp

@@ -1398,6 +1409,100 @@ event_delay_app_instruction(void *drcontext, void *tag, instrlist_t *bb, instr_t
    return DR_EMIT_DEFAULT;
 }

+


How about putting this new function wrapping code in a separate file? The C style here would be a separate file with an init and exit routine. Looks like it needs to share per_thread_t and BUF_PTR -- though I would say just expose an append_marker routine and that can be the start of a future public interface.

derekbruening · 2018-06-19T17:05:09Z

clients/drcachesim/tracer/tracer.cpp

+typedef struct _func_metadata {
+    const trace_marker_type_t marker_retaddr;
+    const trace_marker_type_t marker_arg;
+    const trace_marker_type_t marker_retval;


See above: I think this can all be removed and all we have is an index identifier

derekbruening · 2018-06-19T17:07:40Z

clients/drcachesim/tracer/tracer.cpp

+
+#ifdef UNIX
+    {MALLOC_RETADDR, MALLOC_ARG, MALLOC_RETVAL, "tc_malloc", 1 },
+    {MALLOC_RETADDR, MALLOC_ARG, MALLOC_RETVAL, "__libc_malloc", 1},


Intercepting malloc & co. is a complex topic. Xref Dr. Memory's interception of all heap allocation routines (https://github.com/DynamoRIO/drmemory/blob/master/common/alloc.c#L317,
https://github.com/DynamoRIO/drmemory/blob/master/common/alloc.c#L433) and longstanding plans to turn that into a library (DynamoRIO/drmemory#824) though that has yet to happen.

Please include comments explaining that this is a first pass and is not complete.

derekbruening · 2018-06-19T17:13:04Z

clients/drcachesim/tracer/tracer.cpp

+
+#ifdef UNIX
+    {MALLOC_RETADDR, MALLOC_ARG, MALLOC_RETVAL, "tc_malloc", 1 },
+    {MALLOC_RETADDR, MALLOC_ARG, MALLOC_RETVAL, "__libc_malloc", 1},


I think we want to be able to dynamically specify which functions to record: perhaps via a config file eventually, though we could start with a runtime option. We just need the name and number of args if all we support is recording pointer-sized arg values. You could imagine sthg like:

-record_function malloc 1 -record_function realloc 2

If we decide heap allocation is very common and we want to have built-in support, I would say it should be under a separate option like -record_function_malloc or -record_heap_allocs or sthg.

Alternatively, we could expose an interface to write metadata into the trace and structure all of this as a separate client from the memory address tracer. I'm fine bundling it in for now as it seems like a common thing to want to augment an address trace with.

derekbruening · 2018-06-19T17:23:08Z

Nits about the commit message:

The title: missing #, and as a multi-commit feature please follow the style guide for code reviews. So sthg like i#3048 record funcs: infrastructure and malloc if this retains the hardcoded malloc.

The body: include a ref to the issue: Issue: #3048

…ns to record function Separate out the module of function traces from tracer.cpp to func_trace.cpp. Add burst_malloc test and count number of function markers. Add option to specify record function. Fixes DynamoRIO#3048

Louis-Ye · 2018-06-20T16:45:56Z

clients/drcachesim/tracer/func_trace.cpp

+    int arg_num;
+} func_metadata_t;
+
+/* expected pattern "<function_name>:<function_id>:<arguments_num>" */


Will update this comment with more details.

s-kanev · 2018-06-20T21:26:00Z

clients/drcachesim/tests/burst_malloc.cpp

+                   "-client_lib ';;-offline'"))
+        std::cerr << "failed to set env var!\n";
+
+    /* We use an outer loop to test re-attaching (i#2157). */


leftover comment from copy/paste?

s-kanev · 2018-06-20T21:27:13Z

clients/drcachesim/tests/burst_malloc.cpp

+
+/* 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[])


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

s-kanev · 2018-06-21T16:37:43Z

clients/drcachesim/tracer/func_trace.cpp

+static int func_trace_init_count;
+
+static func_trace_append_entry_t append_entry;
+static std::vector<func_metadata_t> funcs;


Note, about using std::vector (and the std::string above) in a DR client (instead of something like drvector_t). Because in our use-case we are statically linked with the application, you have to be very careful not to break isolation. For example, if func_pre_hook() was calling funcs.push_back(), that could call the same malloc we are currently tracing, which is definitely something we don't want to do, and will lead to all kinds of trouble.

I think you're fine in this case (but @derekbruening sure knows better) -- you're only indexing in the vector when we are called from tracing code. Probably still worth leaving a short comment for the next reader.

This is a very good point, I think very likely it will happen. I will update to use drvector_t and avoid to use std::string in the function hook.

s-kanev · 2018-06-21T16:38:19Z

clients/drcachesim/tracer/func_trace.cpp

+    }
+}
+
+static std::vector<std::string> split_by(std::string s, std::string sep)


nit: formatting static std::vector<> \n

s-kanev · 2018-06-21T16:42:58Z

clients/drcachesim/tracer/func_trace.h

+#ifdef __cplusplus
+extern "C" {
+DR_EXPORT bool
+func_trace_init(func_trace_append_entry_t append_entry_);


Duplicating the declarations this way is confusing. I think it's better to just add the #ifdef ... extern "C" for each of the two below.

Use drvector_t to replace vector in pre/post function hooks. Use customized separator to split option value. Fixes DynamoRIO#3048

Add newline at the end of file to fix style. Fixes DynamoRIO#3048

derekbruening

Had a chance to look at part of the diff

derekbruening · 2018-06-20T20:52:01Z

clients/drcachesim/common/options.cpp

@@ -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
+// XXX: default value should use the customized seperator when the relevant PR comes in


spelling: separator

derekbruening · 2018-06-22T16:04:57Z

clients/drcachesim/common/options.cpp

@@ -333,3 +333,15 @@ 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


Add the issue number (XXX i#3048:)

derekbruening · 2018-06-22T16:06:38Z

clients/drcachesim/common/options.cpp

+ std::string("malloc|0|1") + ";" + std::string("tc_malloc|1|1") + ";" +
+ std::string("__libc_malloc|2|1"),
+ "Record invocation traces for the specified functions",
+ "Option value should fit this format"


Repeating comment from before: please repeat the description in the long (please see the prior comment on the prior review)

derekbruening · 2018-06-22T16:19:21Z

clients/drcachesim/common/options.cpp

+// XXX: replace function return address with function callstack
+droption_t<std::string> op_record_function
+(DROPTION_SCOPE_FRONTEND, "record_function", DROPTION_FLAG_ACCUMULATE, ";",
+ std::string("malloc|0|1") + ";" + std::string("tc_malloc|1|1") + ";" +


Hmm, so by default every trace will include the values for these 3 functions -- not sure about that for a couple of reasons:

It seems incomplete: someone mapping addresses to data structures or sthg will want at least free() as well, if not calloc and new, etc. Yet there's no comment here or anything in the option doc saying these default values are in-progress and not final (yes, a pet peeve of mine: I like prototype or incremental code to clearly mark itself).

Should this be in all address traces by default: I guess compared to the trace overhead the wrapping overhead should be minimal and the trace size expansion should be minimal (should we measure?) But how would we arrange to have a heap tracing option: some kind of alias convenience option -record_heap which turns into -record_function malloc...free...calloc (or it could set up the recording internally after the option parsing).

I think -record_heap sounds like a good idea, I will make such an option and make the default -record_function value as empty.

derekbruening · 2018-06-22T16:22:25Z

clients/drcachesim/common/options.cpp

+ "function_name|function_id|function_args_num (example:"
+ " -record_function \"tc_malloc|2|1\"). 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.");


Explain that the information in the trace is labeled with the function_id via an ID entry prior to each set of value entries.

derekbruening · 2018-06-22T16:24:31Z

clients/drcachesim/common/trace_entry.h

@@ -191,6 +191,32 @@ typedef enum {
     */
    TRACE_MARKER_TYPE_CPU_ID,

+    /**
+     * The marker value contains the function id defined by user in droption


s/user/the user/

Missing .

derekbruening · 2018-06-22T16:28:10Z

clients/drcachesim/common/trace_entry.h

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


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)

derekbruening · 2018-06-22T16:29:06Z

clients/drcachesim/common/trace_entry.h

+    /**
+     * The marker value contains the return address of the function call,
+     * whose id is specified by the closest previous FUNC_ID marker entry
+     * XXX: replace return address with callstack information in the future


This is a comment to us, not the user, so put it in a separate regular (non-doxygen) comment (add the i# too)

derekbruening · 2018-06-22T16:30:00Z

clients/drcachesim/common/trace_entry.h

+     * 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.


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

derekbruening · 2018-06-22T16:30:31Z

clients/drcachesim/common/trace_entry.h

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


the return value, and turn FUNC_ID into a link

derekbruening

Finished rest of PR.

derekbruening · 2018-06-22T16:33:31Z

clients/drcachesim/tests/burst_malloc.cpp

+ */
+
+/* This application links in drmemtrace_static and acquires a trace during
+ * a "burst" of execution in the middle of the application.  It then detaches.


Explain that it invokes malloc to test function tracing

derekbruening · 2018-06-22T16:36:47Z

clients/drcachesim/tests/offline-burst_malloc.templatex

+.*
+all done
+.*
+.* 5000 function markers.*


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.

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. :)

derekbruening · 2018-06-22T16:38:55Z

clients/drcachesim/tools/basic_counts.cpp

@@ -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];


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.

derekbruening · 2018-06-22T16:39:30Z

clients/drcachesim/tracer/drmemtrace.h

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


Repeating prior comment (!): there is no app2app

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

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.

derekbruening · 2018-06-22T16:51:06Z

clients/drcachesim/tracer/func_trace.cpp

+{
+    func_metadata_t *f =
+        (func_metadata_t *)dr_global_alloc(sizeof(func_metadata_t));
+    strcpy(f->name, name.c_str());


Never use strcpy (see https://github.com/DynamoRIO/dynamorio/wiki/Code-Content#security): use strncpy with BUFFER_SIZE_ELEMENTS

derekbruening · 2018-06-22T17:03:36Z

clients/drcachesim/tracer/func_trace.cpp

+    for (auto &single_op_value : split_by(value, sep)) {
+        auto items = split_by(single_op_value, PATTERN_SEPARATOR);
+        if (items.size() != 3) {
+            DR_ASSERT(false);


This is user input so instead of an assert this should be a usage error with a message to the user: the FATAL() macro: FATAL("Usage error: -record_function was not passed a triplet") or sthg.

Used NOTIFY for detectiong format and id duplication problem.
Done

derekbruening · 2018-06-22T17:04:12Z

clients/drcachesim/tracer/func_trace.cpp

+        int id = atoi(items[1].c_str());
+        int arg_num = atoi(items[2].c_str());
+        if (id_existed(id)) {
+            DR_ASSERT(false);


FATAL again with a useful message.

Done with using NOTIFY

derekbruening · 2018-06-22T17:05:41Z

clients/drcachesim/tracer/func_trace.h

+#endif  // __cplusplus
+
+DR_EXPORT
+/**


This is an internal function to the tracer, not exposed to the user: it should not have a doxygen comment, it does not need to be extern "C". Ditto for _exit.

derekbruening · 2018-06-22T17:06:10Z

clients/drcachesim/tracer/tracer.cpp

@@ -152,6 +154,16 @@ static volatile bool exited_process;
 static bool have_phys;
 static physaddr_t physaddr;

+/**


This should be a regular comment: it's not docs to some user interface

derekbruening · 2018-06-22T17:07:02Z

clients/drcachesim/tracer/tracer.cpp

+ * function pre/post callbacks of drwrap API happens before memtrace's
+ * meta instruction, so that function trace entries will not be appended to the middle
+ * of a BB's PC and Memory Access trace entries. (Assumption made here is that,
+ * the function pre/post callback always happended at the first instruction of a BB)


Ready for next round of review.

Add -record_heap for convenient usage of recording heap functions. Update comments to be more professional. Count different function markers separately in basic_counts.cpp/h Fixes DynamoRIO#3048

…ecord_heap shows Add separator for when both -record_function and -record_heap shows Fixes DynamoRIO#3048

Use &as item separator for func options. Modify process_fatal to take only const char*. Fixes DynamoRIO#3048

Notify user about the duplicated ID or invalid input for function trace. Relayout code for ID detection. Remove the passing of fatal_process function pointer. Fixes DynamoRIO#3048

Update comments Fixes DynamoRIO#3048

Louis-Ye · 2018-06-25T21:13:07Z

clients/drcachesim/tracer/func_trace.cpp

+{
+    func_metadata_t *f =
+        (func_metadata_t *)dr_global_alloc(sizeof(func_metadata_t));
+    size_t safe_size = get_safe_size(sizeof(f->name), name.size());


Will update to use BUFFER_SIZE_ELEMENTS(f->name) in next commit.

Use BUFFER_SIZE_ELEMENTS for strncpy. Fixes DynamoRIO#3048

derekbruening

Please reply to all the review comments each time to ensure they are all either acted on or deliberately not acted on (with a reason given): I'm seeing several instances here of review requests that were skipped.
I'm assuming that was a mistake and not deliberate.

Mostly I have comments about the user-facing doxygen docs. The code is good.

derekbruening · 2018-06-25T20:00:43Z

clients/drcachesim/tracer/func_trace.cpp

+{
+    func_metadata_t *f =
+        (func_metadata_t *)dr_global_alloc(sizeof(func_metadata_t));
+    size_t safe_size = get_safe_size(sizeof(f->name), name.size());


Use BUFFER_SIZE_ELEMENTS (to avoid bugs in char vs wchar_t)

derekbruening · 2018-06-26T05:06:33Z

clients/drcachesim/common/options.cpp

+ " 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"


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

derekbruening · 2018-06-26T05:08:20Z

clients/drcachesim/common/options.cpp

+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"


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

derekbruening · 2018-06-26T05:08:33Z

clients/drcachesim/common/options.cpp

+(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"
+ " functions in -record_heap_value. Specify this option is equivalent to"


grammar: s/Specify/Specifying/

derekbruening · 2018-06-26T05:14:48Z

clients/drcachesim/common/options.cpp

+ "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.",


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.)

derekbruening · 2018-06-26T05:26:00Z

clients/drcachesim/common/trace_entry.h

+     * 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


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".

derekbruening · 2018-06-26T05:26:37Z

clients/drcachesim/common/trace_entry.h

+     * 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


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

derekbruening · 2018-06-26T05:36:40Z

clients/drcachesim/tracer/func_trace.h

+    (void *drcontext, trace_marker_type_t marker, uintptr_t value);
+
+DR_EXPORT
+/**


Repeating another prior comment (!): this should be a regular comment.
Also, no DR_EXPORT.

derekbruening · 2018-06-26T05:36:54Z

clients/drcachesim/tracer/func_trace.h

+func_trace_init(func_trace_append_entry_t append_entry_);
+
+DR_EXPORT
+/**


Ditto and ditto.

derekbruening · 2018-06-26T05:39:03Z

clients/drcachesim/tracer/func_trace.cpp

+        return;
+
+    std::string funcs_str, sep;
+    get_funcs_str_and_sep(funcs_str, sep);


I think it's best to cache the function string during init and avoid this parsing here: there are scenarios where using STL code at exit can cause problems (not in google3 usage, but in the dr_app_take_over() scenario where the app exits before DR).

Done. Ready for next round of review!

Save input functions string to avoid using STL at func_trace_exit(). Update comments and docs, correct grammar and spelling issues. Fixes DynamoRIO#3048

Get rid of DR_EXPORT in func_trace.cpp Fixes DynamoRIO#3048

derekbruening · 2018-06-26T19:14:56Z

clients/drcachesim/common/options.cpp

 " Note that the provided function id should be unique, and not collide with"
- " existing heap functions (see -record_heap_value ) if record_heap"
+ " existing heap functions (see -record_heap_value ) if -record_heap"


Repeating (!):
nit: space before )

derekbruening · 2018-06-26T19:31:20Z

clients/drcachesim/common/trace_entry.h

@@ -192,29 +192,32 @@ typedef enum {
    TRACE_MARKER_TYPE_CPU_ID,

    /**
-     * The marker value contains function id defined by the user in droption.
+     * The marker value contains function id defined by the user in the


Repeating (!):
grammar: s/function/the function/

derekbruening · 2018-06-26T19:31:48Z

clients/drcachesim/common/trace_entry.h

     */
    TRACE_MARKER_TYPE_FUNC_ARG,

    /**
-     * The marker value contains return value of the function call,
+     * The marker value contains return value of the just-entered function,


Repeating (!):
grammar: s/return/the return/

Update comments for grammar and format issues. Fixes DynamoRIO#3048

Update comments for consistent format. Fixes DynamoRIO#3048

derekbruening

LGTM. Thank you for updating the coding approach, I like the split heap option and scalable approach.

Thank you for fixing all of the nits (though please carefully look at each review comment in the future: it seems when there were 2 requests in one comment one of them was sometimes overlooked...)

derekbruening · 2018-06-26T20:14:22Z

Looks like it fails to build on Windows. Temporarily adding another -V to suite/runsuite_wrapper.pl will get more info.

Add -V to runsuite_wrapper.pl Fixes DynamoRIO#3048

Add -VV to show more logs. Fixes DynamoRIO#3048

Add explicit cast for fixing windows build. Fixes DynamoRIO#3048

Change -VV back to -V in runsuite_wrapper.pl Fixes DynamoRIO#3048

Louis-Ye · 2018-06-27T00:01:52Z

The Windows build passes now.

derekbruening · 2018-06-27T01:26:27Z

run aarch tests

Louis-Ye added 3 commits June 18, 2018 17:49

i#3048 memtrace-func: add option to enable func trace

f99373e

Add option to enable func trace. Make drwrap init at beginning and exit at the end. Fixes DynamoRIO#3048

i#3048 memtrace-func: check return value of drmgr_register

093cb57

Fixes DynamoRIO#3048

s-kanev reviewed Jun 19, 2018

View reviewed changes

derekbruening requested changes Jun 19, 2018

View reviewed changes

derekbruening closed this Jun 19, 2018

derekbruening reopened this Jun 19, 2018

Louis-Ye changed the title ~~i3048 drcachesim func traces~~ i#3048 drcachesim func traces Jun 19, 2018

Louis-Ye changed the title ~~i#3048 drcachesim func traces~~ i#3048 drcachesim record funcs: infrastructure and malloc Jun 19, 2018

derekbruening mentioned this pull request Jun 19, 2018

Record key function traces in drcachesim #3048

Closed

Louis-Ye commented Jun 20, 2018

View reviewed changes

s-kanev reviewed Jun 21, 2018

View reviewed changes

Louis-Ye added 4 commits June 21, 2018 13:11

Merge branch 'master' into i3048-drcachesim-func-traces

d1713b9

Merge branch 'master' into i3048-drcachesim-func-traces

844b9a3

i#3048 memtrace-func: use drvector_t to replace vector

bb1ead5

Use drvector_t to replace vector in pre/post function hooks. Use customized separator to split option value. Fixes DynamoRIO#3048

i#3048 memtrace-func: fix style by add newline at end of file

32a8f98

Add newline at the end of file to fix style. Fixes DynamoRIO#3048

derekbruening reviewed Jun 22, 2018

View reviewed changes

Louis-Ye added 4 commits June 22, 2018 14:22

Merge branch 'master' into i3048-drcachesim-func-traces

647ecec

i#3048 memtrace-func: add -record_heap, update comments, count markers

acba3bd

Add -record_heap for convenient usage of recording heap functions. Update comments to be more professional. Count different function markers separately in basic_counts.cpp/h Fixes DynamoRIO#3048

i#3048 memtrace-func: add separator when both -record_function and -r…

871c03c

…ecord_heap shows Add separator for when both -record_function and -record_heap shows Fixes DynamoRIO#3048

i#3048 memtrace-func: Use & as item separator

6ec4a13

Use &as item separator for func options. Modify process_fatal to take only const char*. Fixes DynamoRIO#3048

Louis-Ye mentioned this pull request Jun 23, 2018

static DR re-attach extension support #3065

Open

Louis-Ye changed the title ~~i#3048 drcachesim record funcs: infrastructure and malloc~~ i#3048 drcachesim record funcs: infrastructure Jun 23, 2018

Louis-Ye added 2 commits June 24, 2018 14:44

i#3048 memtrace-func: notify instead of crash on duplicate id

23e725c

Notify user about the duplicated ID or invalid input for function trace. Relayout code for ID detection. Remove the passing of fatal_process function pointer. Fixes DynamoRIO#3048

i#3048 memtrace-func: update comments

f1ba45b

Update comments Fixes DynamoRIO#3048

Louis-Ye commented Jun 25, 2018

View reviewed changes

i#3048 memtrace-func: use BUFFER_SIZE_ELEMENTS

0c170a9

Use BUFFER_SIZE_ELEMENTS for strncpy. Fixes DynamoRIO#3048

derekbruening reviewed Jun 26, 2018

View reviewed changes

Louis-Ye added 3 commits June 26, 2018 11:23

Merge branch 'master' into i3048-drcachesim-func-traces

fafa638

i#3048 memtrace-func: save input func str, update comments and docs

90fc899

Save input functions string to avoid using STL at func_trace_exit(). Update comments and docs, correct grammar and spelling issues. Fixes DynamoRIO#3048

i#3048 memtrace-func: get rid of DR_EXPORT

341f812

Get rid of DR_EXPORT in func_trace.cpp Fixes DynamoRIO#3048

derekbruening requested changes Jun 26, 2018

View reviewed changes

Louis-Ye added 2 commits June 26, 2018 12:42

i#3048 memtrace-func: update comments

468d837

Update comments for grammar and format issues. Fixes DynamoRIO#3048

i#3048 memtrace-func: update comments

1943e10

Update comments for consistent format. Fixes DynamoRIO#3048

derekbruening approved these changes Jun 26, 2018

View reviewed changes

Louis-Ye added 4 commits June 26, 2018 13:37

i#3048 memtrace-func: add -V to runsuite_wrapper.pl

d635d36

Add -V to runsuite_wrapper.pl Fixes DynamoRIO#3048

i#3048 memtrace-func: add -VV

d358bf2

Add -VV to show more logs. Fixes DynamoRIO#3048

i#3048 memtrace-func: add explicit cast for windows build

8d6c0a2

Add explicit cast for fixing windows build. Fixes DynamoRIO#3048

i#3048 memtrace-func: change -VV back to -V

4f45cdf

Change -VV back to -V in runsuite_wrapper.pl Fixes DynamoRIO#3048

Merge branch 'master' into i3048-drcachesim-func-traces

0f64c4b

derekbruening merged commit 5492fa3 into DynamoRIO:master Jun 27, 2018

s-kanev mentioned this pull request Jul 2, 2018

drsyms: Support configuring without libelftc #3080

Closed

		@@ -1398,6 +1409,100 @@ event_delay_app_instruction(void drcontext, void tag, instrlist_t *bb, instr_t
		return DR_EMIT_DEFAULT;
		}

i#3048 drcachesim record funcs: infrastructure #3057

i#3048 drcachesim record funcs: infrastructure #3057

Conversation

Louis-Ye commented Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbruening commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekbruening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Louis-Ye commented Jun 19, 2018 •

edited

Loading