Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libomnibus changing weak symbol to undefined? #62

Open
teh opened this issue Dec 30, 2022 · 5 comments
Open

libomnibus changing weak symbol to undefined? #62

teh opened this issue Dec 30, 2022 · 5 comments

Comments

@teh
Copy link

teh commented Dec 30, 2022

I'm building a cxx_python_extension, but when I'm trying to import it I'm getting an undefined symbol:

ImportError: ...buck-out/v2/gen/root/291b6c3a26d6a3e9/__hello__/hello#link-tree/libomnibus.so: undefined symbol: _ITM_registerTMCloneTable

AFAICT this is happens because libomnibus changes w to U. I think the nm command should probably exclude weak symbols (with --no-weak) but I don't understand whether this is by design or a bug?

example output from nm:

ls buck-out/v2/gen/root/291b6c3a26d6a3e9/__hello__/hello#link-tree/*.so | xargs -tn1 nm | rg _ITM_registerTMCloneTable
nm buck-out/v2/gen/root/291b6c3a26d6a3e9/__hello__/hello#link-tree/libc10.so
                 w _ITM_registerTMCloneTable
nm buck-out/v2/gen/root/291b6c3a26d6a3e9/__hello__/hello#link-tree/libomnibus.so
                 U _ITM_registerTMCloneTable
nm buck-out/v2/gen/root/291b6c3a26d6a3e9/__hello__/hello#link-tree/libtorch_cpu.so
                 w _ITM_registerTMCloneTable
nm buck-out/v2/gen/root/291b6c3a26d6a3e9/__hello__/hello#link-tree/libtorch_python.so
                 w _ITM_registerTMCloneTable
@ndmitchell
Copy link
Contributor

I thought the open source code defaulted to turning off Omnibus? We generally don't recommend using omnibus mode. Did you do something to explicitly enable it?

@teh
Copy link
Author

teh commented Jan 2, 2023

I didn't turn it out explicitly but I can reproduce how it got turned on: by depending on an extension that in turn depends on a prebuilt_cxx_library:

mkdir hello
touch hello/main.py
touch hello/main.cc
python_binary(
    name="hello",
    main_module="hello.main",
    deps=[":hello_lib"],
)

python_library(
    name="hello_lib",
    srcs=["hello/main.py"],
    visibility=["PUBLIC"],
    deps=[":hello_ext"],
)

cxx_python_extension(
    name="hello_ext",
    srcs=["hello/main.cc"],
    visibility=["PUBLIC"],
    deps=[":pybind11"],
)

prebuilt_cxx_library(
    name="pybind11",
    header_dirs=["deps/include/"],
    exported_headers=glob(["deps/include/pybind11/**/*.h"]),
    header_only=True,
    visibility=["PUBLIC"],
)

@ndmitchell
Copy link
Contributor

Omnibus is fairly experimental - it has advantages if you have absolutely huge C++ codebases, but normally can be safely turned off. Unfortunately our open source toolchain was defaulting to it being turned on, which was a mistake. I'm just putting up a diff which reads:

--- a/buck2/prelude/toolchains/python.bzl
+++ b/buck2/prelude/toolchains/python.bzl
@@ -62,7 +62,7 @@
             make_pex_inplace = ctx.attrs.make_pex_inplace[RunInfo],
             compile = RunInfo(args = ["echo", "COMPILEINFO"]),
             package_style = "inplace",
-            native_link_strategy = "merged",
+            native_link_strategy = "separate",
         ),
         PythonPlatformInfo(name = "x86_64"),
     ]

Hopefully that resolves the problem by not using Omnibus.

@ndmitchell
Copy link
Contributor

abd3850 applies that change - can you give it another go?

@teh
Copy link
Author

teh commented Jan 4, 2023

I can confirm that stops building libomnibus, thank you! I think the original bug (don't rewrite weak symbols to undefined symbols) is still valid so will leave closing of issue to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants