Issue 319740043: i#2070 burst_replace test fails: avoid pulling libdynamorio.so
DescriptionCommit log for first patchset:
---------------
i#2070 burst_replace test fails: avoid pulling libdynamorio.so
Adds link flag "--as-needed" to avoid pulling libdynamorio.so for
appliations that link static DynamoRIO clients.
Fixes #2070
---------------
Patch Set 1 #
Total comments: 3
Patch Set 2 : move --as-needed #Patch Set 3 : Committed #MessagesTotal messages: 8
https://codereview.appspot.com/319740043/diff/1/make/DynamoRIOConfig.cmake.in File make/DynamoRIOConfig.cmake.in (right): https://codereview.appspot.com/319740043/diff/1/make/DynamoRIOConfig.cmake.in... make/DynamoRIOConfig.cmake.in:1021: _DR_append_property_string(TARGET ${target} LINK_FLAGS "-Wl,--as-needed") I don't think is related to a static *client* which could well need to use libDR -- this should be for static *DR*. Also, the order for --as-needed matters: does it undo --whole-archive? Does it only affect libs after it? I think LINK_FLAGS goes before target_link_libraries (maybe worth a comment on that).
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#2070 burst_replace test fails: avoid pulling libdynamorio.so Adds link flag "--as-needed" to avoid pulling libdynamorio.so for appliations that link static DynamoRIO clients. Fixes #2070 Review-URL: https://codereview.appspot.com/319740043 ---------------
Sign in to reply to this message.
https://codereview.appspot.com/319740043/diff/1/make/DynamoRIOConfig.cmake.in File make/DynamoRIOConfig.cmake.in (right): https://codereview.appspot.com/319740043/diff/1/make/DynamoRIOConfig.cmake.in... make/DynamoRIOConfig.cmake.in:1021: _DR_append_property_string(TARGET ${target} LINK_FLAGS "-Wl,--as-needed") On 2016/11/18 04:33:58, bruening wrote: > I don't think is related to a static *client* which could well need to use libDR > -- this should be for static *DR*. > > Also, the order for --as-needed matters: does it undo --whole-archive? Does it > only affect libs after it? I think LINK_FLAGS goes before target_link_libraries > (maybe worth a comment on that). I put it here is because it seems that use_DynamoRIO_static_client is only used for statically linking DR and client into app., xref comment at https://github.com/DynamoRIO/dynamorio/blob/master/make/DynamoRIOConfig.cmake.... I could put it in configure_DynamoRIO_static, but feel --as-needed is only needed if we pull in any client. Maybe I should put it in configure_DynamoRIO_static. as-needed only works for libraries listed after. The LINK_FLAGS has to comes before target_link_libraries, right? Otherwise, case like --as-needed is useless. For --as-needed from the doc https://sourceware.org/binutils/docs/ld/Options.html For --whole-archive from https://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_node/ld_3.html So --as-needed update the ELF tag, while --whole-archive pull module into archive, seems would work together. Basically, I do not think --whole-archive would put libdynamorio.so into the archive, and --as-needed will not put DT_NEEDED tag for libdynamorio.so.
Sign in to reply to this message.
https://codereview.appspot.com/319740043/diff/1/make/DynamoRIOConfig.cmake.in File make/DynamoRIOConfig.cmake.in (right): https://codereview.appspot.com/319740043/diff/1/make/DynamoRIOConfig.cmake.in... make/DynamoRIOConfig.cmake.in:1021: _DR_append_property_string(TARGET ${target} LINK_FLAGS "-Wl,--as-needed") On 2016/11/18 17:13:35, zhaoqin wrote: > On 2016/11/18 04:33:58, bruening wrote: > > I don't think is related to a static *client* which could well need to use > libDR > > -- this should be for static *DR*. > > I put it here is because it seems that use_DynamoRIO_static_client is only used > for statically linking DR and client into app., xref comment at > https://github.com/DynamoRIO/dynamorio/blob/master/make/DynamoRIOConfig.cmake.... > I could put it in configure_DynamoRIO_static, but feel --as-needed is only > needed if we pull in any client. > Maybe I should put it in configure_DynamoRIO_static. I would vote for configure_DynamoRIO_static. Today, static DR but dynamic clients won't actually work, and static clients but non-static DR might never happen, but it still feels like this is linked to DR being static. There is a risk of this messing up some other libs the app uses: we'll just live with that (we already set all kind of flags that could affect things).
Sign in to reply to this message.
On 2016/11/18 19:27:44, bruening wrote: > https://codereview.appspot.com/319740043/diff/1/make/DynamoRIOConfig.cmake.in > File make/DynamoRIOConfig.cmake.in (right): > > https://codereview.appspot.com/319740043/diff/1/make/DynamoRIOConfig.cmake.in... > make/DynamoRIOConfig.cmake.in:1021: _DR_append_property_string(TARGET ${target} > LINK_FLAGS "-Wl,--as-needed") > On 2016/11/18 17:13:35, zhaoqin wrote: > > On 2016/11/18 04:33:58, bruening wrote: > > > I don't think is related to a static *client* which could well need to use > > libDR > > > -- this should be for static *DR*. > > > > I put it here is because it seems that use_DynamoRIO_static_client is only > used > > for statically linking DR and client into app., xref comment at > > > https://github.com/DynamoRIO/dynamorio/blob/master/make/DynamoRIOConfig.cmake.... > > I could put it in configure_DynamoRIO_static, but feel --as-needed is only > > needed if we pull in any client. > > Maybe I should put it in configure_DynamoRIO_static. > > I would vote for configure_DynamoRIO_static. Today, static DR but dynamic > clients won't actually work, and static clients but non-static DR might never > happen, but it still feels like this is linked to DR being static. > > There is a risk of this messing up some other libs the app uses: we'll just live > with that (we already set all kind of flags that could affect things). agreed, the CL is updated
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
Committed as https://github.com/DynamoRIO/dynamorio/commit/84cb1f51960e2da2d0efea307c2c983... Final commit log: --------------- i#2070 burst_replace test fails: avoid pulling libdynamorio.so Adds link flag "--as-needed" to avoid pulling libdynamorio.so for appliations that link static DynamoRIO clients. Fixes #2070 Review-URL: https://codereview.appspot.com/319740043 ---------------
Sign in to reply to this message.
|
