i#2717 shrink static DR library: do not initialize dynamo_options sta… by nilayvaish · Pull Request #2797 · DynamoRIO/dynamorio

nilayvaish · 2018-01-07T23:58:05Z

…tically.

We currently initialize dynamo_options at the time of declaration. This
is not required since the initialization is same as default_options
variable. Instead, we now copy the default_options to dynamo_options in
the function options_init(). This saves about 27,020 bytes of space.

Before:
-r-xr-xr-x 1 nilayvaish eng 2594134 Nov 24 23:37
blaze-bin/third_party/dynamorio/libdynamorio.a

After:
-r-xr-xr-x 1 nilayvaish eng 2567114 Nov 25 12:32
blaze-bin/third_party/dynamorio/libdynamorio.a

Issue: #2717

…tically. We currently initialize dynamo_options at the time of declaration. This is not required since the initialization is same as default_options variable. Instead, we now copy the default_options to dynamo_options in the function options_init(). This saves about 27,020 bytes of space. Before: -r-xr-xr-x 1 nilayvaish eng 2594134 Nov 24 23:37 blaze-bin/third_party/dynamorio/libdynamorio.a After: -r-xr-xr-x 1 nilayvaish eng 2567114 Nov 25 12:32 blaze-bin/third_party/dynamorio/libdynamorio.a Issue: #2717

fhahn

Looks reasonable to me, thanks!

Could you please shorten the title of the PR a bit? It's too long and parts got spilled in the description.

fhahn · 2018-01-08T10:02:05Z

 # ifdef DEBUG
    /* FIXME: share code w/ main init routine? */
    nonshared_stats.logmask = LOG_ALL;
+    /* Why do we need this to options_init()?  We did so above. */


It does not look like options_init directly depends on anything set by the functions above, but maybe one of the functions it calls does. If this call is really needed, it might be problematic, as it replaces all options set previously by the default options. @derekbruening do you know if it would be safe to drop this call?

Out sick, will try to take a look tomorrow

The -stats_logmask option sets stats->logmask and stats == nonshared_stats so there is a connection. The default logmask has to be set before calling options_init(). I believe this was the original call, and the one above was added later. I think the logmask setting on line 890 needs to be moved up above the options_init() on line 869, and then the options_init() here can be removed.

fhahn · 2018-01-08T10:03:23Z

    /* .lspdata pages start out writable so no unprotect needed here */
    write_lock(&options_lock);
    ASSERT(sizeof(dynamo_options) == sizeof(options_t));
+    /* Set to default options. */


This changes the behavior of options_init. Maybe add a comment to the documentation of the function to make it clear that all previously set options will be overwritten by the default values?

derekbruening · 2018-01-09T19:37:04Z

    options_init();
+#if defined(INTERNAL) && defined(DEADLOCK_AVOIDANCE)
+    /* avoid issues w/ GLOBAL_DCONTEXT instead of thread dcontext */
+    dynamo_options.deadlock_avoidance = false;


Hmm, this now clobbers the final value, so the user cannot override -- I don't think deadlock detection code works for standalone mode anyway, but this new behavior of silently ignoring a requested option is not ideal. Maybe the better solution is to move this into optionsx.h with IF_STANDALONE_ELSE or sthg.

derekbruening · 2018-01-09T20:12:57Z

    write_lock(&options_lock);
    ASSERT(sizeof(dynamo_options) == sizeof(options_t));
+    /* Set to default options. */
+    dynamo_options = default_options;


Seems cleaner to use set_dynamo_options_defaults(), and then remove the call to adjust_defaults_for_page_size() below.

derekbruening · 2018-01-09T20:32:46Z

 # ifdef DEBUG
    /* FIXME: share code w/ main init routine? */
    nonshared_stats.logmask = LOG_ALL;
+    /* Why do we need this to options_init()?  We did so above. */


The -stats_logmask option sets stats->logmask and stats == nonshared_stats so there is a connection. The default logmask has to be set before calling options_init(). I believe this was the original call, and the one above was added later. I think the logmask setting on line 890 needs to be moved up above the options_init() on line 869, and then the options_init() here can be removed.

…i2717-options-init

We currently initialize dynamo_options at the time of declaration. This is not required since the initialization is same as default_options variable. Instead, we now copy the default_options to dynamo_options in the function options_init(). This saves about 27,020 bytes of space. Before: -r-xr-xr-x 1 nilayvaish eng 2594134 Nov 24 23:37 dynamorio/libdynamorio.a After: -r-xr-xr-x 1 nilayvaish eng 2567114 Nov 25 12:32 dynamorio/libdynamorio.a Issue: #2717

derekbruening · 2018-01-16T18:47:20Z

 #endif

 #ifdef DEADLOCK_AVOIDANCE
+# ifdef STANDALONE_UNIT_TEST


Sorry, my suggestion of changing the default here will not work -- this define is only for unit tests, while this needs to be done for standalone mode in general which has no separate define.

…i2717-options-init

Almost everything fails with PR when running on Windows. One of the errors reported was 'access violation'. Restoring the code that only releases the writer lock on options variable. Earlier we had made them readable as well.

nilayvaish · 2018-01-25T08:28:05Z

Currently, with this patch exactly one (non-flaky) test fails: debug-internal-64: linux.execve64. Here is the reason for failure:

63/282 Test #8: linux.execve64 ...............................................***Failed Required regular expression not found.Regex=[^parent is running under DynamoRIO
parent waiting for child
child is running under DynamoRIO
it_worked
running under DynamoRIO
child has exited
$
] 0.78 sec
parent is running under DynamoRIO
parent waiting for child
child is running under DynamoRIO
ERROR: ld.so: object 'libdrpreload.so' from LD_PRELOAD cannot be preloaded (cannot open shared object file): ignored.
it_worked
running under DynamoRIO
child has exited

As can be seen above, the difference is the failure to pre-load libdrpreload.so. I don't know how this change affects the existence of this file. @derekbruening , can you take a look? What am I missing?

derekbruening · 2018-01-26T05:01:13Z

The preload library shouldn't be used by default, so maybe the early inject option is being lost somewhere and it thinks the user requested late injection. You're not able to reproduce the failure locally?

…i2717-options-init

nilayvaish · 2018-01-30T05:33:23Z

No. I am not able to reproduce it locally. I'll try to figure out the control flow and see where inject option is being changed.

Also, thanks for fixing the gcc6 build issues. Earlier, I had to comment out certain portions in the build files to make the build go through. Now, the everything compiles cleanly.

fhahn reviewed Jan 8, 2018

View reviewed changes

derekbruening reviewed Jan 9, 2018

View reviewed changes

nilayvaish added 2 commits January 14, 2018 19:55

Merge branch 'master' of https://github.com/DynamoRIO/dynamorio into …

ce94baf

…i2717-options-init

derekbruening reviewed Jan 16, 2018

View reviewed changes

nilayvaish added 5 commits January 19, 2018 05:22

Merge branch 'master' of https://github.com/DynamoRIO/dynamorio into …

80800ac

…i2717-options-init

Fixes problems due to options not being readable.

60842f2

Use already defined functions for making options readonly and writable.

e4984be

Merge branch 'master' of https://github.com/DynamoRIO/dynamorio into …

25a77b1

…i2717-options-init

Trying to fix the broken tests on Windows.

5c9763e

Almost everything fails with PR when running on Windows. One of the errors reported was 'access violation'. Restoring the code that only releases the writer lock on options variable. Earlier we had made them readable as well.

Merge branch 'master' of https://github.com/DynamoRIO/dynamorio into …

4ae534e

…i2717-options-init

Uh oh!

Conversation

nilayvaish commented Jan 7, 2018

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nilayvaish commented Jan 25, 2018

Uh oh!

derekbruening commented Jan 26, 2018

Uh oh!

nilayvaish commented Jan 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants