i#2006 generalize drcachesim: set drmemtrace dr_client_main as a weak symbol

Commit log for first patchset:
---------------
i#2006 generalize drcachesim: set drmemtrace dr_client_main as a weak symbol

For the case of the application statically linking drmemtrace for
profiling, we want to use drmemtrace as the default client and
support other profiling clients.
We sets dr_client_main in drmemtrace as a weak symbol so that it can
be replaced if necessary by linking another client.
---------------

[zhaoqin, 2016-12-01 16:32:49]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: set drmemtrace dr_client_main as a weak symbol

Adds dr_client_main_memtrace as the real memtrace initialization
function.

Sets dr_client_main as a weak symbol so that it can be replaced in the
case of the application statically linking multiple clients.

Review-URL: https://codereview.appspot.com/317950043
---------------

[bruening, 2016-12-01 22:44:25]

I have several comments about this being very specific and manual and whether it
should be more general and automated: we'll just have to do this again on every
client we want to use statically.  If this is only meant as a first tentative
step to see whether we want to go the weak symbol route, then LGTM if comments
are added about making it more general and automated.

[bruening, 2016-12-01 22:44:32]

https://codereview.appspot.com/317950043/diff/20001/clients/drcachesim/tracer...
File clients/drcachesim/tracer/tracer.cpp (right):

https://codereview.appspot.com/317950043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:813: dr_client_main_memtrace(client_id_t
id, int argc, const char *argv[])
If we need to add a new init routine for each client, should it follow a
stricter naming convention?  This is not "memtrace" (that's a sample client),
this is "drmemtrace".

https://codereview.appspot.com/317950043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:813: dr_client_main_memtrace(client_id_t
id, int argc, const char *argv[])
Hmm, I was hoping for a more general solution where we marked every client's
dr_client_main as WEAK when STATIC_LIBRARY is turned on and that's all it took. 
But I guess there's no simple way to allow calling the preferred one without
adding a separate export for each.

Is there some way to automate adding this?  Are we going to have to manually add
this for every client we want to use statically?

What about third-party clients?  Should we try to set up some automation or at
least a convention so we could use future clients without modifying them?

https://codereview.appspot.com/317950043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:926: DR_EXPORT WEAK void
Should this only be WEAK when being built as static?

https://codereview.appspot.com/317950043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:926: DR_EXPORT WEAK void
What about for future clients?  It would be best if we could use them as-is
without having to go edit them for static use.  Should this WEAK be incorporated
into a DR header and used on all instances of dr_client_main?  

Can it be inside DR_EXPORT if that's only used for dr_client_main?

[zhaoqin, 2016-12-02 16:25:36]

https://codereview.appspot.com/317950043/diff/20001/clients/drcachesim/tracer...
File clients/drcachesim/tracer/tracer.cpp (right):

https://codereview.appspot.com/317950043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:813: dr_client_main_memtrace(client_id_t
id, int argc, const char *argv[])
On 2016/12/01 22:44:32, bruening wrote:
> If we need to add a new init routine for each client, should it follow a
> stricter naming convention?  This is not "memtrace" (that's a sample client),
> this is "drmemtrace".

rename it to drmemtrace_client_main, so the name convention is
clientname_client_main.

https://codereview.appspot.com/317950043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:813: dr_client_main_memtrace(client_id_t
id, int argc, const char *argv[])
On 2016/12/01 22:44:32, bruening wrote:
> Hmm, I was hoping for a more general solution where we marked every client's
> dr_client_main as WEAK when STATIC_LIBRARY is turned on and that's all it
took. 
> But I guess there's no simple way to allow calling the preferred one without
> adding a separate export for each.
> 
> Is there some way to automate adding this?  Are we going to have to manually
add
> this for every client we want to use statically?
> 
> What about third-party clients?  Should we try to set up some automation or at
> least a convention so we could use future clients without modifying them?

As we discussed offline, have two symbol pointing to the same function, one
global symbol drmemtrace_client_main, and one weak symbol dr_client_main would
be nice.
Tried use linker option --defsym=drmemtrace_client_main=dr_client_main, which
works for a shared lib build, but fail at the application build with static
link.
So current solution would work well for both Windows and Linux.
We could investigate more if we are adding more clients.

https://codereview.appspot.com/317950043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:926: DR_EXPORT WEAK void
On 2016/12/01 22:44:32, bruening wrote:
> Should this only be WEAK when being built as static?

Not if it is the only client linked to app, and no other dr_client_main is
implemented.

https://codereview.appspot.com/317950043/diff/20001/clients/drcachesim/tracer...
clients/drcachesim/tracer/tracer.cpp:926: DR_EXPORT WEAK void
On 2016/12/01 22:44:32, bruening wrote:
> What about for future clients?  It would be best if we could use them as-is
> without having to go edit them for static use.  Should this WEAK be
incorporated
> into a DR header and used on all instances of dr_client_main?  
> 
> Can it be inside DR_EXPORT if that's only used for dr_client_main?

As discussed off line, many drext are kind of client and use DR_EXPORT, so it
might not be a good idea to change DR_EXPORT, even just for client.

As for the future clients, as explained above, we could investigate the approach
alias two symbols and use command link to avoid code change.

[zhaoqin, 2016-12-02 16:29:28]

Commit log for latest patchset:
---------------
i#2006 generalize drcachesim: set drmemtrace dr_client_main as a weak symbol

Adds drmemtrace_client_main as the real drmemtrace initialization
function.

Sets dr_client_main as a weak symbol so that it can be replaced in the
case of the application statically linking multiple clients.

Review-URL: https://codereview.appspot.com/317950043
---------------

[zhaoqin, 2016-12-06 13:37:46]

Committed as
https://github.com/DynamoRIO/dynamorio/commit/2f7bebcc36b52a3a321676c6af9b070...

Final commit log: 
---------------
i#2006 generalize drcachesim: set drmemtrace dr_client_main as a weak symbol

Adds drmemtrace_client_main as the real drmemtrace initialization
function.

Sets dr_client_main as a weak symbol so that it can be replaced in the
case of the application statically linking multiple clients.

Review-URL: https://codereview.appspot.com/317950043
---------------