Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2277863002:
Enable loading native libraries with RTLD_DEEPBIND (Closed)
|
Created:
4 years, 3 months ago by Ken Rockot(use gerrit already) Modified:
4 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable loading native libraries with RTLD_DEEPBIND
Using RTLD_DEEPBIND by default is problematic (see comments
in native_library_posix.cc), but in the case of Mojo services
where the host binary is lightweight but otherwise may export some
conflicting symbols (e.g. tcmalloc), it is desirable.
This CL adds a NativeLibraryOptions structure and a new
LoadNativeLibraryWithOptions() call. The only supported
option (|prefer_own_symbols|) only affects behavior on
non-OSX, non-Android, POSIX systems.
The default behavior of LoadNativeLibrary is unchanged.
Also adds a unit test for general native library loading since
there wasn't one before.
BUG=594674
Committed: https://crrev.com/596a0dd73a3b8f8eaacb58526540107efcba5a57
Cr-Commit-Position: refs/heads/master@{#414605}
Patch Set 1 : . #
Total comments: 20
Patch Set 2 : . #
Total comments: 3
Patch Set 3 : Enable loading native libraries with RTLD_DEEPBIND #
Total comments: 1
Patch Set 4 : . #
Total comments: 7
Patch Set 5 : \o/ #
Total comments: 6
Patch Set 6 : . #
Dependent Patchsets: Messages
Total messages: 78 (56 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Enable loading native libraries with RTLD_DEEPBIND Using RTLD_DEEPBIND by default is problematic (see comments in native_library_posix.cc), but in the case of Mojo services where the host binary is lightweight but otherwise may export some conflicting symbols (e.g. tcmalloc), it is desirable. BUG=594674 ========== to ========== Enable loading native libraries with RTLD_DEEPBIND Using RTLD_DEEPBIND by default is problematic (see comments in native_library_posix.cc), but in the case of Mojo services where the host binary is lightweight but otherwise may export some conflicting symbols (e.g. tcmalloc), it is desirable. Also adds a unit tests for general native library loading since there wasn't one before. BUG=594674 ==========
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rockot@chromium.org changed reviewers: + primiano@chromium.org, sky@chromium.org, thestig@chromium.org
thestig@, please take a look - see bug for context. Thanks! +primiano@ +sky@ FYI
Description was changed from ========== Enable loading native libraries with RTLD_DEEPBIND Using RTLD_DEEPBIND by default is problematic (see comments in native_library_posix.cc), but in the case of Mojo services where the host binary is lightweight but otherwise may export some conflicting symbols (e.g. tcmalloc), it is desirable. Also adds a unit tests for general native library loading since there wasn't one before. BUG=594674 ========== to ========== Enable loading native libraries with RTLD_DEEPBIND Using RTLD_DEEPBIND by default is problematic (see comments in native_library_posix.cc), but in the case of Mojo services where the host binary is lightweight but otherwise may export some conflicting symbols (e.g. tcmalloc), it is desirable. This CL adds a NativeLibraryOptions structure and a new LoadNativeLibraryWithOptions() call. The only supported option (|prefer_own_symbols|) only affects behavior on non-OSX, non-Android, POSIX systems. The default behavior of LoadNativeLibrary is unchanged. Also adds a unit tests for general native library loading since there wasn't one before. BUG=594674 ==========
Description was changed from ========== Enable loading native libraries with RTLD_DEEPBIND Using RTLD_DEEPBIND by default is problematic (see comments in native_library_posix.cc), but in the case of Mojo services where the host binary is lightweight but otherwise may export some conflicting symbols (e.g. tcmalloc), it is desirable. This CL adds a NativeLibraryOptions structure and a new LoadNativeLibraryWithOptions() call. The only supported option (|prefer_own_symbols|) only affects behavior on non-OSX, non-Android, POSIX systems. The default behavior of LoadNativeLibrary is unchanged. Also adds a unit tests for general native library loading since there wasn't one before. BUG=594674 ========== to ========== Enable loading native libraries with RTLD_DEEPBIND Using RTLD_DEEPBIND by default is problematic (see comments in native_library_posix.cc), but in the case of Mojo services where the host binary is lightweight but otherwise may export some conflicting symbols (e.g. tcmalloc), it is desirable. This CL adds a NativeLibraryOptions structure and a new LoadNativeLibraryWithOptions() call. The only supported option (|prefer_own_symbols|) only affects behavior on non-OSX, non-Android, POSIX systems. The default behavior of LoadNativeLibrary is unchanged. Also adds a unit test for general native library loading since there wasn't one before. BUG=594674 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
I'm not an owner here. Thanks for figuring this out! https://codereview.chromium.org/2277863002/diff/60001/base/native_library.h File base/native_library.h (right): https://codereview.chromium.org/2277863002/diff/60001/base/native_library.h#n... base/native_library.h:69: public: don't need public https://codereview.chromium.org/2277863002/diff/60001/base/native_library.h#n... base/native_library.h:74: // when |true| corresponds setting RTLD_DEEPBIND on dlopen(). nit: 'setting' -> 'to setting'
Got red bots. I actually haven't been paying full attention to the bug, so I defer to primiano@.
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/24 at 20:28:17, sky wrote: > I'm not an owner here. Thanks for figuring this out! > > https://codereview.chromium.org/2277863002/diff/60001/base/native_library.h > File base/native_library.h (right): > > https://codereview.chromium.org/2277863002/diff/60001/base/native_library.h#n... > base/native_library.h:69: public: > don't need public Done > > https://codereview.chromium.org/2277863002/diff/60001/base/native_library.h#n... > base/native_library.h:74: // when |true| corresponds setting RTLD_DEEPBIND on dlopen(). > nit: 'setting' -> 'to setting' Done
I saw this coming :) Initial pass here, the intent seems good, I'd probably just simplify a bit the code. See comments below. Also plz take a look to RTLD_GLOBAL, not sure whether you need it or not for your purposes. https://codereview.chromium.org/2277863002/diff/120001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2277863002/diff/120001/base/BUILD.gn#newcode1061 base/BUILD.gn:1061: if (use_gold && !is_android) { why use_gold? The linker used to build the binary shouldn't make a different. the behavior is given by the loader, not the linker (unless there is some gold feature I'm not considering). IMHO this should be if (is_linux). But at this point you don't need the define and you can just use #if defined(OS_LINUX) https://codereview.chromium.org/2277863002/diff/120001/base/BUILD.gn#newcode1062 base/BUILD.gn:1062: defines += [ "EXPECT_GLOBAL_RTLD_BEHAVIOR" ] adding defines to an entire target is discouraged, see brett's thread [chromium-dev] New system for feature enable/disable flags [1] If you really want this define, you need to use a buildflag_header here. See allocator:features in base/allocator/BUILD.gn as an example [2] [1] https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5c4ySpPsV14/JJfAe... [2] https://cs.chromium.org/chromium/src/base/allocator/BUILD.gn?q=allocator+feat... https://codereview.chromium.org/2277863002/diff/120001/base/native_library.h File base/native_library.h (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library.h#... base/native_library.h:68: struct BASE_EXPORT NativeLibraryOptions { If there are no other use cases this IMHO should be just an enum or a bitfield (long arg + constants to be or-ed) . https://codereview.chromium.org/2277863002/diff/120001/base/native_library.h#... base/native_library.h:73: // when |true| corresponds to setting RTLD_DEEPBIND on dlopen(). Interestingly it seems that on Mac OSX DEEPBIND is tthe default behavior. I didn't test it but from [1] I read "Note: This doesn't apply to Mac OS* X where the default is the RTLD_DEEPBIND behavior." https://software.intel.com/en-us/articles/ensuring-shared-library-uses-intel-... https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... File base/native_library_mac.mm (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... base/native_library_mac.mm:45: // dlopen() etc. open the file off disk. I think here you want to CHECK that you don't have any options, as those are not supported on mac. https://codereview.chromium.org/2277863002/diff/120001/base/native_library_po... File base/native_library_posix.cc (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library_po... base/native_library_posix.cc:35: flags |= RTLD_DEEPBIND; are you sue you don't need also RTLD_GLOBAL? Can you do a test to you hax toy example, adding an extra library that libhax.so depends on, which calls malloc and see if RTLD_GLOBAL affects binding of that third library? https://codereview.chromium.org/2277863002/diff/120001/base/native_library_un... File base/native_library_unittest.cc (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library_un... base/native_library_unittest.cc:57: "libnative_library_test_library.cr.so"; .cr.so should be only for component builds. Also looks liek you don't support android, why bothering about this test on non supported platforms? https://codereview.chromium.org/2277863002/diff/120001/base/native_library_un... base/native_library_unittest.cc:111: TEST(NativeLibraryTest, LoadLibraryPreferOwnSymbols) { I think it's just clear if this is #ifdef OS_LINUX, not sure EXPECT_GLOBAL_RTLD_BEHAVIOR makes it easier to read.
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! Unsurprisingly, ASAN is unhappy with the intentional ODR violation in the unit test. I've disabled the new tests under ASAN for this reason. https://codereview.chromium.org/2277863002/diff/120001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2277863002/diff/120001/base/BUILD.gn#newcode1061 base/BUILD.gn:1061: if (use_gold && !is_android) { On 2016/08/24 at 22:14:08, Primiano Tucci wrote: > why use_gold? The linker used to build the binary shouldn't make a different. the behavior is given by the loader, not the linker (unless there is some gold feature I'm not considering). > IMHO this should be if (is_linux). > But at this point you don't need the define and you can just use #if defined(OS_LINUX) Well, this was failing on cast_shell_linux, and the only relevant difference I could find between is_chromecast true/false is that we don't use gold for is_chromecast=true. In that configuration, we don't see the default global behavior. https://codereview.chromium.org/2277863002/diff/120001/base/BUILD.gn#newcode1062 base/BUILD.gn:1062: defines += [ "EXPECT_GLOBAL_RTLD_BEHAVIOR" ] On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > adding defines to an entire target is discouraged, see brett's thread [chromium-dev] New system for feature enable/disable flags [1] > If you really want this define, you need to use a buildflag_header here. See allocator:features in base/allocator/BUILD.gn as an example [2] > > > [1] https://groups.google.com/a/chromium.org/d/msg/chromium-dev/5c4ySpPsV14/JJfAe... > [2] https://cs.chromium.org/chromium/src/base/allocator/BUILD.gn?q=allocator+feat... Oops. This was meant to be only on the base_unittests target (fixed). Is it more palatable there given that it's a leaf target? https://codereview.chromium.org/2277863002/diff/120001/base/native_library.h File base/native_library.h (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library.h#... base/native_library.h:68: struct BASE_EXPORT NativeLibraryOptions { On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > If there are no other use cases this IMHO should be just an enum or a bitfield (long arg + constants to be or-ed) . I'd prefer to use a struct here only because it's more easily extensible to non-boolean option types, and because (to me anyway) it seems easier to read and understand code which uses actual types instead of integer bitfields. I don't feel strongly about this though, so if you really want me to change it I will :) https://codereview.chromium.org/2277863002/diff/120001/base/native_library.h#... base/native_library.h:73: // when |true| corresponds to setting RTLD_DEEPBIND on dlopen(). On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > Interestingly it seems that on Mac OSX DEEPBIND is tthe default behavior. > I didn't test it but from [1] I read "Note: This doesn't apply to Mac OS* X where the default is the RTLD_DEEPBIND behavior." > > https://software.intel.com/en-us/articles/ensuring-shared-library-uses-intel-... Added a note about this https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... File base/native_library_mac.mm (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... base/native_library_mac.mm:45: // dlopen() etc. open the file off disk. On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > I think here you want to CHECK that you don't have any options, as those are not supported on mac. I would prefer we silently ignore the option since it's really only a flag to prefer local resolution when not the default behavior. Ideally IMHO user code shouldn't have add platform guards when using this. But then maybe a more accurate name would be force_own_symbols or force_local_resolution or something? https://codereview.chromium.org/2277863002/diff/120001/base/native_library_po... File base/native_library_posix.cc (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library_po... base/native_library_posix.cc:35: flags |= RTLD_DEEPBIND; On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > are you sue you don't need also RTLD_GLOBAL? > Can you do a test to you hax toy example, adding an extra library that libhax.so depends on, which calls malloc and see if RTLD_GLOBAL affects binding of that third library? We may want this behavior but it might be worth keeping that separated from this CL. I'm not sure what behavior is on Windows for example, and I'd like to write and test that change indepedently. We do not currently have any need to load DSOs which are linked against other shared libraries. https://codereview.chromium.org/2277863002/diff/120001/base/native_library_un... File base/native_library_unittest.cc (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library_un... base/native_library_unittest.cc:57: "libnative_library_test_library.cr.so"; On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > .cr.so should be only for component builds. > Also looks liek you don't support android, why bothering about this test on non supported platforms? I suppose we don't need to support it now, but I wouldn't rule out Android support in the future. Doesn't seem like it should be much effort to ensure this behavior just works on Android anyway? https://codereview.chromium.org/2277863002/diff/120001/base/native_library_un... base/native_library_unittest.cc:111: TEST(NativeLibraryTest, LoadLibraryPreferOwnSymbols) { On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > I think it's just clear if this is #ifdef OS_LINUX, not sure EXPECT_GLOBAL_RTLD_BEHAVIOR makes it easier to read. see comment in BUILD.gn
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Alright, well it turns out that I only greened the tests with my use_gold/EXPECT_GLOBAL_RTLD_BEHAVIOR hack because I defined it in the wrong target and completely disabled the relevant test logic :> So of course you're right, gold had nothing to do with it. It doesn't seem worthwhile to try and sort out exactly under which circumstances we should expect the default behavior of global symbol resolution. All that matters is that when the flag is set, we get local resolution. As such, I've removed that unnecessary part of the test as well as the use_gold hack which was added for it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
> Unsurprisingly, ASAN is unhappy with the intentional ODR violation in the unit test. I've disabled the new tests under ASAN for this reason. Uhm do you have a link to the failure? I am a bit surprised that asan cares about the same symbol being defined in two linker units. https://codereview.chromium.org/2277863002/diff/120001/base/native_library.h File base/native_library.h (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library.h#... base/native_library.h:68: struct BASE_EXPORT NativeLibraryOptions { On 2016/08/24 23:23:57, Ken Rockot wrote: > On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > > If there are no other use cases this IMHO should be just an enum or a bitfield > (long arg + constants to be or-ed) . > > I'd prefer to use a struct here only because it's more easily extensible to > non-boolean option types, and because (to me anyway) it seems easier to read and > understand code which uses actual types instead of integer bitfields. I don't > feel strongly about this though, so if you really want me to change it I will :) I leave this to base owners, as I'm not an owner here. No particularly strong opinion, I just tend to keep things as simple as possible unless there is a use case and refactor later if I need something fancier.. https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... File base/native_library_mac.mm (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... base/native_library_mac.mm:45: // dlopen() etc. open the file off disk. On 2016/08/24 23:23:57, Ken Rockot wrote: > On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > > I think here you want to CHECK that you don't have any options, as those are > not supported on mac. > > I would prefer we silently ignore the option since it's really only a flag to > prefer local resolution when not the default behavior. Silently changing ignoring a requested behavior feels a bad API to me. IMHO either you guarantee that such behavior just happens to work that way in windows and mac, in which case is fine to leave it because it happens that "prefer_own_symbols" just works by default on those platforms (but in this case please make sure that the test works). If not you should check. > Ideally IMHO user code > shouldn't have add platform guards when using this. But then maybe a more > accurate name would be force_own_symbols or force_local_resolution or something? If the user code tries to request this feature on a platform where this feature is not available we should IMHO hard crash and make it evident that the user code is doing something wrong. https://codereview.chromium.org/2277863002/diff/180001/base/native_library_un... File base/native_library_unittest.cc (right): https://codereview.chromium.org/2277863002/diff/180001/base/native_library_un... base/native_library_unittest.cc:116: EXPECT_FALSE(library.IsSameTestSymbolAddress(&g_native_library_test_symbol)); I'd probably rework the test to work as follows: 1. - Let the both the DSO and the unittest have an exported method with the same name/signature but which returns two different values. - Add another exported method to the DSO (only) like DoCallTheMethodAbove() { return MethodAbove(); } and EXPECT_EQ that you get the return value of the DSO and not the one from the unittest. 2. Same with a g_exported_variable. - Add an exported g_variable both to the DSO and the test. - Add a Get/SetValue method only to the DSO which does SetValue(v) { g_value = v } - In the unittest do something like: g_value = 41; DSOSetValue(42); EXPECT_EQ(42, g_value); g_value = 43; EXPECT_EQ(43, DSOGetValue()); I would NOT do any pointer comparison. That feels too implementation dependent. I'm not sure if the linker is allowed to create some trampoline aliases. I'd test the behavior instead.
Thanks! No new changes yet, just some replies On 2016/08/25 at 10:20:45, primiano wrote: > > Unsurprisingly, ASAN is unhappy with the intentional ODR violation in the unit test. I've disabled the new tests under ASAN for this reason. > > Uhm do you have a link to the failure? I am a bit surprised that asan cares about the same symbol being defined in two linker units. https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... > > https://codereview.chromium.org/2277863002/diff/120001/base/native_library.h > File base/native_library.h (right): > > https://codereview.chromium.org/2277863002/diff/120001/base/native_library.h#... > base/native_library.h:68: struct BASE_EXPORT NativeLibraryOptions { > On 2016/08/24 23:23:57, Ken Rockot wrote: > > On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > > > If there are no other use cases this IMHO should be just an enum or a bitfield > > (long arg + constants to be or-ed) . > > > > I'd prefer to use a struct here only because it's more easily extensible to > > non-boolean option types, and because (to me anyway) it seems easier to read and > > understand code which uses actual types instead of integer bitfields. I don't > > feel strongly about this though, so if you really want me to change it I will :) > > I leave this to base owners, as I'm not an owner here. No particularly strong opinion, I just tend to keep things as simple as possible unless there is a use case and refactor later if I need something fancier.. > > https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... > File base/native_library_mac.mm (right): > > https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... > base/native_library_mac.mm:45: // dlopen() etc. open the file off disk. > On 2016/08/24 23:23:57, Ken Rockot wrote: > > On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > > > I think here you want to CHECK that you don't have any options, as those are > > not supported on mac. > > > > I would prefer we silently ignore the option since it's really only a flag to > > prefer local resolution when not the default behavior. > Silently changing ignoring a requested behavior feels a bad API to me. > IMHO either you guarantee that such behavior just happens to work that way in windows and mac, in which case is fine to leave it because it happens that "prefer_own_symbols" just works by default on those platforms (but in this case please make sure that the test works). > If not you should check. > > > Ideally IMHO user code > > shouldn't have add platform guards when using this. But then maybe a more > > accurate name would be force_own_symbols or force_local_resolution or something? > If the user code tries to request this feature on a platform where this feature is not available we should IMHO hard crash and make it evident that the user code is doing something wrong. > > https://codereview.chromium.org/2277863002/diff/180001/base/native_library_un... > File base/native_library_unittest.cc (right): > > https://codereview.chromium.org/2277863002/diff/180001/base/native_library_un... > base/native_library_unittest.cc:116: EXPECT_FALSE(library.IsSameTestSymbolAddress(&g_native_library_test_symbol)); > I'd probably rework the test to work as follows: > > 1. > - Let the both the DSO and the unittest have an exported method with the same name/signature but which returns two different values. > - Add another exported method to the DSO (only) like DoCallTheMethodAbove() { return MethodAbove(); } and > EXPECT_EQ that you get the return value of the DSO and not the one from the unittest. > > 2. Same with a g_exported_variable. > - Add an exported g_variable both to the DSO and the test. > - Add a Get/SetValue method only to the DSO which does SetValue(v) { g_value = v } > - In the unittest do something like: > g_value = 41; > DSOSetValue(42); > EXPECT_EQ(42, g_value); > g_value = 43; > EXPECT_EQ(43, DSOGetValue()); > > I would NOT do any pointer comparison. That feels too implementation dependent. I'm not sure if the linker is allowed to create some trampoline aliases. I'd test the behavior instead.
https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... File base/native_library_mac.mm (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... base/native_library_mac.mm:45: // dlopen() etc. open the file off disk. On 2016/08/25 at 10:20:44, Primiano Tucci wrote: > On 2016/08/24 23:23:57, Ken Rockot wrote: > > On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > > > I think here you want to CHECK that you don't have any options, as those are > > not supported on mac. > > > > I would prefer we silently ignore the option since it's really only a flag to > > prefer local resolution when not the default behavior. > Silently changing ignoring a requested behavior feels a bad API to me. > IMHO either you guarantee that such behavior just happens to work that way in windows and mac, in which case is fine to leave it because it happens that "prefer_own_symbols" just works by default on those platforms (but in this case please make sure that the test works). > If not you should check. > > > Ideally IMHO user code > > shouldn't have add platform guards when using this. But then maybe a more > > accurate name would be force_own_symbols or force_local_resolution or something? > If the user code tries to request this feature on a platform where this feature is not available we should IMHO hard crash and make it evident that the user code is doing something wrong. If we were actually going to make the feature unavailable on other platforms, I'd rather restrict the definition itself to only those platforms. In any case this requires all to write noise like #if defined(OS...) options.prefer_own_symbols = true; #endif which seems unfortunate to me when the meaning of the flag seems justifiable even without a corresponding OS feature: the flag is defined to have no effect when |false|; and when |true|, it forces behavior that is already the default on some platforms. The test exists to ensure that this is the case. https://codereview.chromium.org/2277863002/diff/180001/base/native_library_un... File base/native_library_unittest.cc (right): https://codereview.chromium.org/2277863002/diff/180001/base/native_library_un... base/native_library_unittest.cc:116: EXPECT_FALSE(library.IsSameTestSymbolAddress(&g_native_library_test_symbol)); On 2016/08/25 at 10:20:44, Primiano Tucci wrote: > I'd probably rework the test to work as follows: > > 1. > - Let the both the DSO and the unittest have an exported method with the same name/signature but which returns two different values. > - Add another exported method to the DSO (only) like DoCallTheMethodAbove() { return MethodAbove(); } and > EXPECT_EQ that you get the return value of the DSO and not the one from the unittest. Ah, yeah I considered that as well. I'll do this. > > 2. Same with a g_exported_variable. > - Add an exported g_variable both to the DSO and the test. > - Add a Get/SetValue method only to the DSO which does SetValue(v) { g_value = v } > - In the unittest do something like: > g_value = 41; > DSOSetValue(42); > EXPECT_EQ(42, g_value); > g_value = 43; > EXPECT_EQ(43, DSOGetValue()); Now I'm confused. This test would seem to verify the behavior we *don't* want. What we want this flag to ensure is: g_value = 41; DSOSetValue(42); EXPECT_EQ(41, g_value); g_value = 43; EXPECT_EQ(42, DSOGetValue()); > > I would NOT do any pointer comparison. That feels too implementation dependent. I'm not sure if the linker is allowed to create some trampoline aliases. I'd test the behavior instead.
https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... File base/native_library_mac.mm (right): https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... base/native_library_mac.mm:45: // dlopen() etc. open the file off disk. On 2016/08/25 15:52:12, Ken Rockot wrote: > On 2016/08/25 at 10:20:44, Primiano Tucci wrote: > > On 2016/08/24 23:23:57, Ken Rockot wrote: > > > On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > > > > I think here you want to CHECK that you don't have any options, as those > are > > > not supported on mac. > > > > > > I would prefer we silently ignore the option since it's really only a flag > to > > > prefer local resolution when not the default behavior. > > Silently changing ignoring a requested behavior feels a bad API to me. > > IMHO either you guarantee that such behavior just happens to work that way in > windows and mac, in which case is fine to leave it because it happens that > "prefer_own_symbols" just works by default on those platforms (but in this case > please make sure that the test works). > > If not you should check. > > > > > Ideally IMHO user code > > > shouldn't have add platform guards when using this. But then maybe a more > > > accurate name would be force_own_symbols or force_local_resolution or > something? > > If the user code tries to request this feature on a platform where this > feature is not available we should IMHO hard crash and make it evident that the > user code is doing something wrong. > > > If we were actually going to make the feature unavailable on other platforms, > I'd rather restrict the definition itself to only those platforms. In any case > this requires all to write noise like > > #if defined(OS...) > options.prefer_own_symbols = true; > #endif > > which seems unfortunate to me when the meaning of the flag seems justifiable > even without a corresponding OS feature: the flag is defined to have no effect > when |false|; and when |true|, it forces behavior that is already the default on > some platforms. The test exists to ensure that this is the case. > Oh hold on, I think we might be saying the same thing. Let me try to reword it: if we can guarantee that on the other platforms (where we ignore the flag) true corresponds with the default behavior I agree we don't need any #ifdef. My only concern is: if we cannot support it, let's not silently ignore it. But from your comments sounds like we are already in the state where the other platforms are fine (%android where I'd CHECK(false)) https://codereview.chromium.org/2277863002/diff/180001/base/native_library_un... File base/native_library_unittest.cc (right): https://codereview.chromium.org/2277863002/diff/180001/base/native_library_un... base/native_library_unittest.cc:116: EXPECT_FALSE(library.IsSameTestSymbolAddress(&g_native_library_test_symbol)); On 2016/08/25 15:52:12, Ken Rockot wrote: > On 2016/08/25 at 10:20:44, Primiano Tucci wrote: > > I'd probably rework the test to work as follows: > > > > 1. > > - Let the both the DSO and the unittest have an exported method with the same > name/signature but which returns two different values. > > - Add another exported method to the DSO (only) like DoCallTheMethodAbove() { > return MethodAbove(); } and > > EXPECT_EQ that you get the return value of the DSO and not the one from the > unittest. > > Ah, yeah I considered that as well. I'll do this. > > > > > 2. Same with a g_exported_variable. > > - Add an exported g_variable both to the DSO and the test. > > - Add a Get/SetValue method only to the DSO which does SetValue(v) { g_value > = v } > > - In the unittest do something like: > > g_value = 41; > > DSOSetValue(42); > > EXPECT_EQ(42, g_value); > > g_value = 43; > > EXPECT_EQ(43, DSOGetValue()); > > Now I'm confused. This test would seem to verify the behavior we *don't* want. Oh, you are right. I screwed up my example. That is the current behavior not the one you are introducing. > What we want this flag to ensure is: > > g_value = 41; > DSOSetValue(42); > EXPECT_EQ(41, g_value); > g_value = 43; > EXPECT_EQ(42, DSOGetValue()); Right, this one. Ack!
On 2016/08/25 at 16:14:33, primiano wrote: > https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... > File base/native_library_mac.mm (right): > > https://codereview.chromium.org/2277863002/diff/120001/base/native_library_ma... > base/native_library_mac.mm:45: // dlopen() etc. open the file off disk. > On 2016/08/25 15:52:12, Ken Rockot wrote: > > On 2016/08/25 at 10:20:44, Primiano Tucci wrote: > > > On 2016/08/24 23:23:57, Ken Rockot wrote: > > > > On 2016/08/24 at 22:14:09, Primiano Tucci wrote: > > > > > I think here you want to CHECK that you don't have any options, as those > > are > > > > not supported on mac. > > > > > > > > I would prefer we silently ignore the option since it's really only a flag > > to > > > > prefer local resolution when not the default behavior. > > > Silently changing ignoring a requested behavior feels a bad API to me. > > > IMHO either you guarantee that such behavior just happens to work that way in > > windows and mac, in which case is fine to leave it because it happens that > > "prefer_own_symbols" just works by default on those platforms (but in this case > > please make sure that the test works). > > > If not you should check. > > > > > > > Ideally IMHO user code > > > > shouldn't have add platform guards when using this. But then maybe a more > > > > accurate name would be force_own_symbols or force_local_resolution or > > something? > > > If the user code tries to request this feature on a platform where this > > feature is not available we should IMHO hard crash and make it evident that the > > user code is doing something wrong. > > > > > > If we were actually going to make the feature unavailable on other platforms, > > I'd rather restrict the definition itself to only those platforms. In any case > > this requires all to write noise like > > > > #if defined(OS...) > > options.prefer_own_symbols = true; > > #endif > > > > which seems unfortunate to me when the meaning of the flag seems justifiable > > even without a corresponding OS feature: the flag is defined to have no effect > > when |false|; and when |true|, it forces behavior that is already the default on > > some platforms. The test exists to ensure that this is the case. > > > > Oh hold on, I think we might be saying the same thing. > Let me try to reword it: if we can guarantee that on the other platforms (where we ignore the flag) true corresponds with the default behavior I agree we don't need any #ifdef. > My only concern is: if we cannot support it, let's not silently ignore it. But from your comments sounds like we are already in the state where the other platforms are fine (%android where I'd CHECK(false)) I probably missed something, but why should we CHECK(false) on Android? The test actually does pass, so although the NDK doesn't define RTLD_DEEPBIND, it seems to be the default behavior. > > https://codereview.chromium.org/2277863002/diff/180001/base/native_library_un... > File base/native_library_unittest.cc (right): > > https://codereview.chromium.org/2277863002/diff/180001/base/native_library_un... > base/native_library_unittest.cc:116: EXPECT_FALSE(library.IsSameTestSymbolAddress(&g_native_library_test_symbol)); > On 2016/08/25 15:52:12, Ken Rockot wrote: > > On 2016/08/25 at 10:20:44, Primiano Tucci wrote: > > > I'd probably rework the test to work as follows: > > > > > > 1. > > > - Let the both the DSO and the unittest have an exported method with the same > > name/signature but which returns two different values. > > > - Add another exported method to the DSO (only) like DoCallTheMethodAbove() { > > return MethodAbove(); } and > > > EXPECT_EQ that you get the return value of the DSO and not the one from the > > unittest. > > > > Ah, yeah I considered that as well. I'll do this. > > > > > > > > 2. Same with a g_exported_variable. > > > - Add an exported g_variable both to the DSO and the test. > > > - Add a Get/SetValue method only to the DSO which does SetValue(v) { g_value > > = v } > > > - In the unittest do something like: > > > g_value = 41; > > > DSOSetValue(42); > > > EXPECT_EQ(42, g_value); > > > g_value = 43; > > > EXPECT_EQ(43, DSOGetValue()); > > > > Now I'm confused. This test would seem to verify the behavior we *don't* want. > > Oh, you are right. I screwed up my example. That is the current behavior not the one you are introducing. > > > What we want this flag to ensure is: > > > > g_value = 41; > > DSOSetValue(42); > > EXPECT_EQ(41, g_value); > > g_value = 43; > > EXPECT_EQ(42, DSOGetValue()); > > Right, this one. Ack!
Patchset #5 (id:200001) has been deleted
Patchset #3 (id:160001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #3 (id:220001) has been deleted
Reworked the test and added a CHECK for Android https://codereview.chromium.org/2277863002/diff/240001/base/native_library_un... File base/native_library_unittest.cc (right): https://codereview.chromium.org/2277863002/diff/240001/base/native_library_un... base/native_library_unittest.cc:126: EXPECT_EQ(42, library.Call<int>("GetMagicValue")); Only problem now: If prefer_own_symbols is false, this check still passes (the ones above do NOT, as expected). So this means the test is somehow not quite emulating the malloc behavior we want to test.
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Patchset #4 (id:260001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
OK! I believe I'm now modeling the correct symbol arrangement to sufficiently test this behavior. It seems to be important for the executable target and the shared library to get their duplicate exports from the same object file in order to fully exercise the undesirable default behavior, so I've moved the duplicate exports to a new source_set and updated dependencies accordingly. I have verified that without the new option set on a local Linux build, all appropriate expectations fail in the test. And of course, with the option set, everything passes. Note that this is still disabled for ASAN; its ODR complaint is regarding only the g_native_library_exported_value symbol. It doesn't care about the duplicate NativeLibraryTestIncrement(), which should explain why we don't get similar complaints about malloc et al. While the test covers interesting usage of this symbol, I think we actually get all the test coverage we need in practice from NativeLibraryTestIncrement(). So we could remove the Get/SetExportedValue test code and turn this on for ASAN, or we could just leave it disabled for ASAN. I'd lean toward leaving it disabled since there otherwise isn't much interesting going on here for ASAN to catch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Excellent, thanks a lot. LGTM with some small final comments (I am not an owner of //base though). > Note that this is still disabled for ASAN; its ODR complaint is regarding only the g_native_library_exported_value symbol. It doesn't care about the duplicate NativeLibraryTestIncrement(), which should explain why we don't get similar complaints about malloc et al. Ahh now makes sense, thanks for looking into it! > So we could remove the Get/SetExportedValue test code and turn this on for ASAN, or we could just leave it disabled for ASAN. > While the test covers interesting usage of this symbol, I think we actually get all the test coverage we need in practice from NativeLibraryTestIncrement(). Hmm I'd honestly lean more to keep the test on the explicit g_native_library_exported_value. In practice that would give you coverage for cases where you have a flag which is set/get via an inline function (so the setter is not trampolined via PLT, but the variable itself needs to be trampolined via GOT). I agree with disabling the test on ASAN. P.S. Good luck with NaCl. Every time I landed a CL like this it got reverted because it broke some mysterious bot running a mysterious NaCl configuration. May the force be with you. :P https://codereview.chromium.org/2277863002/diff/280001/base/native_library.h File base/native_library.h (right): https://codereview.chromium.org/2277863002/diff/280001/base/native_library.h#... base/native_library.h:69: NativeLibraryOptions() {} didn't catchup with C++11 too much. Is the copy ctor of this working as intended? Don't you need a =default copy ctor? https://codereview.chromium.org/2277863002/diff/280001/base/native_library_po... File base/native_library_posix.cc (right): https://codereview.chromium.org/2277863002/diff/280001/base/native_library_po... base/native_library_posix.cc:34: // Android dlopen() behavior is not well specified and may vary across I'd make this a bit softer and say: "dlopen() behavior on Android requires further investigation, as it might vary across versions. Crash here..." https://codereview.chromium.org/2277863002/diff/280001/base/native_library_un... File base/native_library_unittest.cc (right): https://codereview.chromium.org/2277863002/diff/280001/base/native_library_un... base/native_library_unittest.cc:61: const NativeLibraryOptions& options = NativeLibraryOptions()) isn't this a reference to a temporary object? No idea how this plays with default argument. If unsure stay conservative and pass by copy :) https://codereview.chromium.org/2277863002/diff/280001/base/native_library_un... base/native_library_unittest.cc:78: GetFunctionPointerFromNativeLibrary(library_, function_name))(args...); neat :)
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2277863002/diff/280001/base/native_library.h File base/native_library.h (right): https://codereview.chromium.org/2277863002/diff/280001/base/native_library.h#... base/native_library.h:69: NativeLibraryOptions() {} On 2016/08/25 at 21:10:53, Primiano Tucci wrote: > didn't catchup with C++11 too much. Is the copy ctor of this working as intended? Don't you need a =default copy ctor? It's not necessary, but given the confusion I added an explicit default copy ctor. Seems harmless imho. https://codereview.chromium.org/2277863002/diff/280001/base/native_library_po... File base/native_library_posix.cc (right): https://codereview.chromium.org/2277863002/diff/280001/base/native_library_po... base/native_library_posix.cc:34: // Android dlopen() behavior is not well specified and may vary across On 2016/08/25 at 21:10:53, Primiano Tucci wrote: > I'd make this a bit softer and say: "dlopen() behavior on Android requires further investigation, as it might vary across versions. Crash here..." Done https://codereview.chromium.org/2277863002/diff/280001/base/native_library_un... File base/native_library_unittest.cc (right): https://codereview.chromium.org/2277863002/diff/280001/base/native_library_un... base/native_library_unittest.cc:61: const NativeLibraryOptions& options = NativeLibraryOptions()) On 2016/08/25 at 21:10:53, Primiano Tucci wrote: > isn't this a reference to a temporary object? No idea how this plays with default argument. > If unsure stay conservative and pass by copy :) Nah, this will default-construct a new value at the site of every call which uses the default argument.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2277863002/diff/300001/base/native_library_ma... File base/native_library_mac.mm (right): https://codereview.chromium.org/2277863002/diff/300001/base/native_library_ma... base/native_library_mac.mm:43: const NativeLibraryOptions& options, Do we want to DCHECK() |options| and see if anyone is trying to set it here? Ditto for Windows. Or is that a bit of a pain for callers of LoadNativeLibraryWithOptions() ? If you went over this already, I'll admit again that I haven't been paying attention. https://codereview.chromium.org/2277863002/diff/300001/base/native_library_un... File base/native_library_unittest.cc (right): https://codereview.chromium.org/2277863002/diff/300001/base/native_library_un... base/native_library_unittest.cc:61: const NativeLibraryOptions& options = NativeLibraryOptions()) Can we instead have a separate TestLibrary() ctor that just delegates to this ctor, and remove the default argument. https://codereview.chromium.org/2277863002/diff/300001/base/native_library_un... base/native_library_unittest.cc:83: }; DISALLOW_COPY_AND_ASSIGN ?
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2277863002/diff/300001/base/native_library_ma... File base/native_library_mac.mm (right): https://codereview.chromium.org/2277863002/diff/300001/base/native_library_ma... base/native_library_mac.mm:43: const NativeLibraryOptions& options, On 2016/08/25 at 23:08:58, Lei Zhang wrote: > Do we want to DCHECK() |options| and see if anyone is trying to set it here? Ditto for Windows. Or is that a bit of a pain for callers of LoadNativeLibraryWithOptions() ? > > If you went over this already, I'll admit again that I haven't been paying attention. Indeed, we went over this already :) To summarize: We have a CHECK on android since we're not sure of the behavior of dlopen() there. On other platforms where the DEEPBIND behavior is the default, the flag is allowed but has no effect. The option is defined in such a way that it should be neither confusing nor semantically incorrect when setting it has no net effect. https://codereview.chromium.org/2277863002/diff/300001/base/native_library_un... File base/native_library_unittest.cc (right): https://codereview.chromium.org/2277863002/diff/300001/base/native_library_un... base/native_library_unittest.cc:61: const NativeLibraryOptions& options = NativeLibraryOptions()) On 2016/08/25 at 23:08:58, Lei Zhang wrote: > Can we instead have a separate TestLibrary() ctor that just delegates to this ctor, and remove the default argument. Done https://codereview.chromium.org/2277863002/diff/300001/base/native_library_un... base/native_library_unittest.cc:83: }; On 2016/08/25 at 23:08:58, Lei Zhang wrote: > DISALLOW_COPY_AND_ASSIGN ? Done
On 2016/08/25 23:20:43, Ken Rockot wrote: > Indeed, we went over this already :) To summarize: :) Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rockot@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2277863002/#ps320001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Enable loading native libraries with RTLD_DEEPBIND Using RTLD_DEEPBIND by default is problematic (see comments in native_library_posix.cc), but in the case of Mojo services where the host binary is lightweight but otherwise may export some conflicting symbols (e.g. tcmalloc), it is desirable. This CL adds a NativeLibraryOptions structure and a new LoadNativeLibraryWithOptions() call. The only supported option (|prefer_own_symbols|) only affects behavior on non-OSX, non-Android, POSIX systems. The default behavior of LoadNativeLibrary is unchanged. Also adds a unit test for general native library loading since there wasn't one before. BUG=594674 ========== to ========== Enable loading native libraries with RTLD_DEEPBIND Using RTLD_DEEPBIND by default is problematic (see comments in native_library_posix.cc), but in the case of Mojo services where the host binary is lightweight but otherwise may export some conflicting symbols (e.g. tcmalloc), it is desirable. This CL adds a NativeLibraryOptions structure and a new LoadNativeLibraryWithOptions() call. The only supported option (|prefer_own_symbols|) only affects behavior on non-OSX, non-Android, POSIX systems. The default behavior of LoadNativeLibrary is unchanged. Also adds a unit test for general native library loading since there wasn't one before. BUG=594674 Committed: https://crrev.com/596a0dd73a3b8f8eaacb58526540107efcba5a57 Cr-Commit-Position: refs/heads/master@{#414605} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/596a0dd73a3b8f8eaacb58526540107efcba5a57 Cr-Commit-Position: refs/heads/master@{#414605} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
