Repository navigation
Statically-linked libraries in TF binary can cause symbol collisions #9525
Description
Activity
I'm trying to solve this in #9391 . Follow up there.
@nkhdiscovery @drpngx Should we dedup this with #9391?
@nkhdiscovery We're planning to fix this for good using more restricted exports. In particular, protos should not appear in the public API once we're done. Do you mind if I close that other bug?
@girving Thanks for closing that one, I would be happy if I could help. I already tried hiding symbols with adding version scripts to cc project but I couldn't figure a clean way to write the regex matching unnecessary symbols. I would do that if you give me a hint on what are the API functions to make them public (global) and make else hidden (local) (_TF* as didn't work as global), or a hint on how to hide symbols coming from specific headers. I Googled a lot and found almost nothing good enough on using version scripts to solve this.
Version scripts are not so good for C++ based projects, because it's hard to control what needs to be public and not. This is way visibility attributes were added. Everything, except symbol versions are available as attributes :( Versions script is a good starting point (I would prefer to have symbol versions).
Reacted by Navid Khazaee (nkh)We'll be marking exported symbols with a TF_EXPORT macro, but there's a bunch of upfront work to do to minimize the API surface area before we do that.
Reacted by Navid Khazaee (nkh)@davidlt Are you suggesting to put
__attribute__ ((visibility ("default")))in the code wherever there is something to hide? Because that requires lots of additions in third_party headers, e.g. protobuf itself, which will cause further expenses if one of those third_party libraries planned to be upgraded. I just didn't think of that as a solution because I felt it's just making another mess, even re-packing those libraries with new names and dynamically link against those specific versions seems much more cleaner to me.@girving First, as I understood from your comment, this TF_EXPORT macro will then help us to recognize what has to be exported and what is not to, right? I mean at least I will be able to put every single function which has that macro in my version script and temporarily solve the problem for my own usage. Am I right?
Second, I see you have to do this later to avoid the redundant work (putting the macro wherever it is needed and then removing that whole part of API in minimization will not be logical, for sure); But if this is the only problem for not doing that now, I can do that redundant work in a fork or a temporary branch so we can use the remained parts after minimization. I have to solve this for myself as soon as possible, so let's do it in a way which is helpful for further contribution. Can you give me any hint on how to do that? I saw the usage in tensorflow/core/framework/types.h in master, I think I can give a try if I know what exactly has to be exported.Thanks for your replies guys.
@nkhdiscovery Once we're done, the code will be compiled with
-fvisibility=hidden, and only symbols marked withTF_EXPORTwill be exported. Unfortunately I don't understand the details of versions scripts, so I don't know enough to answer what you can do that will be useful. Most of the work is restructuring the actual code to make it easy to restrict exports, not doing the actual export restriction.@girving Thanks for your answer, I just understood what you are doing as the solution. Is there any way I can contribute to accelerate this? Isn't it just enough to add this macro to all API functions?
Most of the complexity is refactoring the code so that protos don't need to be exposed, since we don't have control over those. I'm not sure how to parallelize the required refactoring, and unfortunately a good chunk of the complexity is making sure said refactoring doesn't break non-opensource code.
Reacted by Navid Khazaee (nkh)25 remaining items
@nkhdiscovery the next step IMO would be to split off a shared object with the implementations of our protocol buffers (libtensorflow_protobufs.so?).
The main benefit would be that users of the C++ API would no longer need to link against libtensorflow_framework.so (or build libtensorflow_cc statically) for protocol buffer symbols, and so would run into fewer symbol conflicts. So basically #14267; it's closed at the moment, but you could re-open it and work on it. We have workarounds but no great solution for C++ API users who want to use OpenCV and use custom ops (custom ops won't work with static libtensorflow_cc, OpenCV won't work with dynamic libtensorflow_cc).
There are two things to be moved: one is the static variables for protocol buffer registration (
@protobuf_archive//:protobuf), the other is the generated implementations of TensorFlow's protocol buffers. My thought is that these should stay together for now.Steps I think the split would include:
- Hacking around with build rules until
bazel query 'somepath(//tensorflow:libtensorflow_framework.so, @protobuf_archive//:protobuf)'andbazel query 'somepath(//tensorflow:libtensorflow_framework.so, //tensorflow/core:protos_all_cc_impl)'return empty results - Include these explicitly in a new //tensorflow:libtensorflow_protobuf.so rule (near libtensorflow_framework.so), and include
//tensorflow:libtensorflow_protobuf.soin tf_binary_additional_srcs. - Make sure all the tests pass :). Easy to have undefined symbols when messing with linking.
- The final step would be dealing with packaging issues, such as for the Java bindings (which I believe still hard-code the names of TensorFlow libraries).
Happy to chat more if this sounds interesting. Sending an email to [email protected] with a rough plan and discussing would be a good start (@gunan and others are working on a related effort, so coordinating would be important).
- Hacking around with build rules until
@JosephIWB
Doing a monolithic build disables the ability to hide gpu devices from tensorflow session,
How are you hiding them? CUDA_VISIBLE_DEVICES? I have no idea why this wouldn't work, but if you have a quick repro someone can take a look.
There's also the "add yet another shared object" workaround for the OpenCV symbol conflict.
Oh I see, the issue is that C++ API doesn't include protobuf symbols. You need to link against libtensorflow_framework.so for those (unfortunately a known issue).
Do you think the fvisibility change is submittable? May be worth running the tests (e.g. bazel test -c opt //tensorflow/core/... //tensorflow/python/...), and if they pass making a pull request out of it (I'm happy to review). If we don't need the symbols which conflict with OpenCV, we should stop exporting them.
I tried running the tests suggested by @allenlavoie . Unfortunately, my workaround breaks the test build. k8-py3-opt/bin/_solib_local/libtensorflow_Score_Slibjpeg_Uinternal.so needs those jpeg symbols exported.
external/local_config_cuda/crosstool/clang/bin/crosstool_wrapper_driver_is_not_gcc -o bazel-out/k8-py3-opt/bin/tensorflow/core/grappler/costs/utils_test '-Wl,-rpath,$ORIGIN/../../../../_solib_local/' '-Wl,-rpath,$ORIGIN/../../../../_solib_local/_U_S_Stensorflow_Score_Sgrappler_Scosts_Cutils_Utest___Utensorflow' '-Wl,-rpath,$ORIGIN/../../../../_solib_local/_U@local_Uconfig_Ucuda_S_Scuda_Ccublas___Uexternal_Slocal_Uconfig_Ucuda_Scuda_Scuda_Slib' '-Wl,-rpath,$ORIGIN/../../../../_solib_local/_U@local_Uconfig_Ucuda_S_Scuda_Ccusolver___Uexternal_Slocal_Uconfig_Ucuda_Scuda_Scuda_Slib' '-Wl,-rpath,$ORIGIN/../../../../_solib_local/_U@local_Uconfig_Ucuda_S_Scuda_Ccudart___Uexternal_Slocal_Uconfig_Ucuda_Scuda_Scuda_Slib' -Lbazel-out/k8-py3-opt/bin/_solib_local/_U_S_Stensorflow_Score_Sgrappler_Scosts_Cutils_Utest___Utensorflow -Lbazel-out/k8-py3-opt/bin/_solib_local -Lbazel-out/k8-py3-opt/bin/_solib_local/_U@local_Uconfig_Ucuda_S_Scuda_Ccublas___Uexternal_Slocal_Uconfig_Ucuda_Scuda_Scuda_Slib -Lbazel-out/k8-py3-opt/bin/_solib_local/_U@local_Uconfig_Ucuda_S_Scuda_Ccusolver___Uexternal_Slocal_Uconfig_Ucuda_Scuda_Scuda_Slib -Lbazel-out/k8-py3-opt/bin/_solib_local/_U@local_Uconfig_Ucuda_S_Scuda_Ccudart___Uexternal_Slocal_Uconfig_Ucuda_Scuda_Scuda_Slib '-Wl,-rpath,$ORIGIN/,-rpath,$ORIGIN/..,-rpath,$ORIGIN/../..,-rpath,$ORIGIN/../../..' -Wl,-z,muldefs -Wl,-z,notext -Wl,-z,notext -Wl,-z,notext -Wl,-z,notext -Wl,-z,notext -Wl,-z,notext -Wl,-z,notext -pthread -Wl,-z,notext -Wl,-z,notext -Wl,-z,notext -Wl,-z,notext -Wl,-z,notext -Wl,-z,notext -Wl,-z,notext -Wl,-rpath,../local_config_cuda/cuda/lib64 -Wl,-rpath,../local_config_cuda/cuda/extras/CUPTI/lib64 -pthread -Wl,-no-as-needed -B/usr/bin/ -pie -Wl,-z,relro,-z,now -no-canonical-prefixes -pass-exit-codes '-Wl,--build-id=md5' '-Wl,--hash-style=gnu' -Wl,--gc-sections -Wl,@bazel-out/k8-py3-opt/bin/tensorflow/core/grappler/costs/utils_test-2.params)
bazel-out/k8-py3-opt/bin/_solib_local/libtensorflow_Score_Slibjpeg_Uinternal.so: undefined reference tojpeg_abort' bazel-out/k8-py3-opt/bin/_solib_local/libtensorflow_Score_Slibjpeg_Uinternal.so: undefined reference tojpeg_set_defaults'
...Thanks @ruanjiandong! I guess not super surprising, but was worth a try. So the options are still (1) split out proto symbols so people don't need to link in libtensorflow_framework.so, (2) move libjpeg to the language bindings / colocated with the kernel. Possibly (2) is easier?
@allenlavoie , I took another look at the build failure. Those tests actually need jpeg_* symbols from libjpeg.so, not libtensorflow_framework.so. Without my change, bazel will produce both static and dynamic libjpeg library for test build.
I made a new change which use ld version script to selectively hide jpeg symbols when linking libtensorflow_framework.so. With the new change, all the tests passed except for 3 grpc tests (related to my test environment). The new change works only for Linux. For OS X, I don't know how to selectively hide symbols using "-exported_symbols_list" option.
I will create a pull request for the new change.
@allenlavoie , could you please review pull request pull #19966 ?
- added a commit that references this issue
on Feb 25, 2021 - added a commit that references this issue
on Apr 24, 2021 @ruanjiandong do you know if something similar can be done to llvm symbols?
Reacted by Dylan Wragge
TensorFlow currently statically links all dependencies. This sometimes causes hard-to-diagnose crashes (e.g. segfaults) when another version of a dependency is loaded into the process. This can even happen within TensorFlow if separate TensorFlow .so's are loaded into the same Python process.
Possible solutions would be to reduce the visibility of these symbols, dynamically link common libraries, or run TF in a separate process.
Known problematic libraries:
Other related issues: