fixed DR-i#1486

drmemory/frontend.c

 /* **********************************************************
  * Copyright (c) 2010-2014 Google, Inc.  All rights reserved.
  * Copyright (c) 2010 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
 /* Dr. Memory: the memory debugger
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
  * License as published by the Free Software Foundation;
@@ skipping 169 matching lines @@
     if (uname(&uinfo) != 0)
         return false;
 #   define MIN_DARWIN_VERSION_SUPPORTED 11  /* OSX 10.7.x */
 #   define MAX_DARWIN_VERSION_SUPPORTED 13  /* OSX 10.9.x */
     return (dr_sscanf(uinfo.release, "%d", &kernel_major) == 1 &&
             kernel_major <= MAX_DARWIN_VERSION_SUPPORTED &&
             kernel_major >= MIN_DARWIN_VERSION_SUPPORTED);
 }
 #endif /* WINDOWS */
 
+static const char *
+dr_config_status_to_msg(dr_config_status_t status)

[bruening, 2014/08/06 23:19:46]

Do you think drconfiglib should provide this routine?  Maybe XXX or file issue
if don't want to think about it now.

[zhaoqin, 2014/08/08 19:18:03]

hmm, actually, I should add it to drconfiglib now.

+{
+    const char *msg;
+    switch (status) {
+    case DR_SUCCESS:
+        /* Operation succeeded. */
+        msg = "successful";
+        break;
+    case DR_PROC_REG_EXISTS:
+        /* Process registration failed due to an existing registration. */
+        msg = "registration already exists";
+        break;
+    case DR_PROC_REG_INVALID:
+        /* Operation failed because the supplied process is not registered. */
+        msg = "target process is not registerred";

[bruening, 2014/08/06 23:19:46]

spelling

[zhaoqin, 2014/08/08 19:18:03]

Done.

+        break;
+    case DR_PRIORITY_INVALID:
+        /* Client registration failed due to an invalid priority value. */
+        msg = "invalid priority value";
+        break;
+    case DR_ID_CONFLICTING:
+        /* Client registration failed due to a conflicting ID. */
+        msg = "conflict id";

[bruening, 2014/08/06 23:19:46]

s/conflict/conflicting/

[zhaoqin, 2014/08/08 19:18:03]

Done.

+        break;
+    case DR_ID_INVALID:
+        /* Client operation failed due to an invalid client ID. */
+        msg = "invalid client ID";
+        break;
+    case DR_NUDGE_PID_NOT_INJECTED:
+        /* Nudge operation failed because the specified process id is not under DR. */
+        msg = "target process is not under Dr.Memory";

[bruening, 2014/08/06 23:19:46]

s/Dr.M/Dr. M/

[zhaoqin, 2014/08/08 19:18:04]

replace it with DynamoRIO since moving it to DynamoRIO, any client could use it.

+        break;
+    case DR_NUDGE_TIMEOUT:
+        /* Nudge operation timed out waiting for target process to finish handling a nudge.*/

[bruening, 2014/08/06 23:19:46]

style

[bruening, 2014/08/06 23:19:46]

(IMHO these comments could all be removed from here, not adding much)

[zhaoqin, 2014/08/08 19:18:03]

Done.

[zhaoqin, 2014/08/08 19:18:03]

Done.

+        msg = "timed out";
+        break;
+    case DR_CONFIG_STRING_TOO_LONG:
+        /* Field length exceeded, probably due to a too-long option string */
+        msg = "config option string too long";
+        break;
+    case DR_CONFIG_FILE_WRITE_FAILED:
+        /* Failed to write to the config file. */
+        msg = "failed to write to the config file";

[bruening, 2014/08/06 23:19:46]

Perhaps this should also say "check permissions on XXX" -- hmm do we have an
easy way to get name of config file or basedir?

[zhaoqin, 2014/08/08 19:18:04]

For moving to DynamoRIO, I think we can only say "failed to write to the config
file".

+        break;
+    case DR_NUDGE_PID_NOT_FOUND:
+        /* Nudge operation failed because the specified process id does not exist. */
+        msg = "target process id does not exist";
+        break;
+    case DR_FAILURE:
+    default:
+        msg = "unknown failure, check caller permissions for this operation";

[bruening, 2014/08/06 23:19:46]

"caller"?  Does the user know anything about "caller"?

[zhaoqin, 2014/08/08 19:18:03]

just keep unknown error.

+        break;
+    }
+    return msg;
+}
+
 static const char *prefix = PREFIX_DEFAULT_MAIN_THREAD;
 
 static void
 pause_if_in_cmd(void)
 {
 #ifdef WINDOWS
     if (!batch &&
         (dr_using_console() ||
          /* i#1157: on win8 dr_using_console() always returns false, so we
           * always pause unless -batch
@@ skipping 737 matching lines @@
     bool exit0 = false;
     bool dr_logdir_specified = false;
     bool doubledash_present = false;
 
 #ifdef WINDOWS
     time_t start_time, end_time;
 #endif
 
     drfront_status_t sc;
     bool res;
+    dr_config_status_t status;
 
     if (dr_standalone_init() == NULL) {
         /* We assume this is due to a new version of Windows */
         fatal("this version of Windows is not supported by Dr. Memory.");
     }
 
 #ifdef WINDOWS
     /* i#1377: we can't trust GetVersionEx() b/c it pretends 6.3 (Win8.1) is
      * 6.2 (Win8)!  Thus we use DR's version.
      */
@@ skipping 328 matching lines @@
             BUFPRINT(client_ops, BUFFER_SIZE_ELEMENTS(client_ops),
                      cliops_sofar, len, "`%s` ", argv[i]);
         }
     }
 
 #ifndef MACOS /* XXX i#1286: implement nudge on MacOS */
     if (nudge_pid != 0) {
         if (i < argc)
             usage("%s", "-nudge does not take an app to run");
         /* could also complain about other client or app specific ops */
         res = dr_nudge_pid(nudge_pid, DRMEM_CLIENT_ID, NUDGE_LEAK_SCAN, INFINITE);

[bruening, 2014/08/06 23:19:46]

Is this a bug b/c "res" is the wrong type?

[zhaoqin, 2014/08/08 19:18:04]

good catch.

         if (res != DR_SUCCESS) {
-            fatal("error nudging %d%s", nudge_pid,
-                  (res == DR_NUDGE_PID_NOT_INJECTED) ? ": no such Dr. Memory process"
-                  : "");
+            const char *err_msg = dr_config_status_to_msg(res);
+            fatal("error nudging %d, error code %d (%s)", nudge_pid, res, err_msg);
             assert(false); /* shouldn't get here */
         }
         exit(0);
     }
 #endif
 
     if (i >= argc)
         usage("%s", "no app specified");
     app_name = argv[i];
     get_full_path(app_name, full_app_name, BUFFER_SIZE_ELEMENTS(full_app_name));
@@ skipping 207 matching lines @@
 
     /* we need to locate the results file, but only for top-level process (i#328) */
     BUFPRINT(client_ops, BUFFER_SIZE_ELEMENTS(client_ops),
              cliops_sofar, len, "-resfile %d ", pid);
 
     process = dr_inject_get_image_name(inject_data);
     /* we don't care if this app is already registered for DR b/c our
      * this-pid config will override
      */
     info("configuring %s pid=%d dr_ops=\"%s\"", process, pid, dr_ops);
-    if (dr_register_process(process, pid,
-                            false/*local*/, dr_root,  DR_MODE_CODE_MANIPULATION,
-                            use_dr_debug, DR_PLATFORM_DEFAULT, dr_ops) != DR_SUCCESS) {
-        fatal("failed to register DynamoRIO configuration");
+    status = dr_register_process(process, pid,
+                                 false/*local*/, dr_root,  DR_MODE_CODE_MANIPULATION,
+                                 use_dr_debug, DR_PLATFORM_DEFAULT, dr_ops);
+    if (status != DR_SUCCESS) {
+        const char *err_msg = dr_config_status_to_msg(status);
+        fatal("%d (%s)\nFailed to register DynamoRIO configuration for \"%s\"(%d) "

[bruening, 2014/08/06 23:19:46]

Hmm, so this will read sthg like "ERROR: 3 (failed to write to the config
file)\nFailed...".  Hmm.  OK.  But maybe better if it's more like the one above
that has the "failed to register..." first, and then has "error code %d (%s)"?

[zhaoqin, 2014/08/08 19:18:03]

ok, I used to put the error at the end, but the line is too long, kind of hard
to find the error code, add \n should solve the problem.

+              "dr_ops=\"%s\"",
+              status, err_msg, process, pid, dr_ops);
         goto error; /* actually won't get here */
     }
     info("configuring client \"%s\" ops=\"%s\"", client_path, client_ops);
-    if (dr_register_client(process, pid,
-                           false/*local*/, DR_PLATFORM_DEFAULT, DRMEM_CLIENT_ID,
-                           0, client_path, client_ops) != DR_SUCCESS) {
-        fatal("failed to register DynamoRIO client configuration");
+    status = dr_register_client(process, pid,
+                                false/*local*/, DR_PLATFORM_DEFAULT, DRMEM_CLIENT_ID,
+                                0, client_path, client_ops);
+    if (status != DR_SUCCESS) {
+        const char *err_msg = dr_config_status_to_msg(status);
+        fatal("%d (%s)\nFailed to register DynamoRIO client configuration for \"%s\", "
+              "ops=\"%s\"", status, err_msg, client_path, client_ops);

[bruening, 2014/08/06 23:19:46]

Ditto

[zhaoqin, 2014/08/08 19:18:03]

Done.

         goto error; /* actually won't get here */
     }
 
     if (native_parent) {
         /* Create a regular config file without -native_parent so the children will
          * run normally.
          */
         info("configuring child processes");
         if (dr_process_is_registered(process, 0, false/*local*/, DR_PLATFORM_DEFAULT,
                                      NULL, NULL, NULL, NULL)) {
@@ skipping 64 matching lines @@
     if (sc != DRFRONT_SUCCESS)
         fatal("failed to free memory for args: %d", sc);
     if (errcode != 0) {
         /* We use a prefix to integrate better with tool output, esp inside
          * the VS IDE as an External Tool.
          */
         warn_prefix("application exited with abnormal code 0x%x", errcode);
     }
     return (exit0 ? 0 : errcode);
 }