Transitive cc_shared_library doesn't work with inline functions

[brians-neptune]

Description of the bug:

cc_shared_library and dynamic_deps deliberately link the shared libraries for only direct dependencies, while excluding the corresponding static libraries transitively. If any of the direct dependencies have inline functions which use transitive dependencies, this can result in linking failures. This is common with C++ code, it's less common but I think it's still possible with C.

Having to specify all the transitive dependencies in dynamic_deps is not good. It's even worse because sometimes with some compilers and some optimization settings it's necessary, and other times it's not (it depends on what gets inlined where).

Linking all of the transitive shared libraries seems low-cost. They're going to be needed at runtime anyways. Would that be an acceptable change?

Which category does this issue belong to?

C++ Rules

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

https://github.com/brians-neptune/bazel-cc_shared_library-transitive-deps has a main_dynamic target which fails to link, but I think it should. main_static is the same thing without dynamic_deps, which does link.

Which operating system are you running Bazel on?

Ubuntu 22.04

What is the output of bazel info release?

release 7.0.2

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

No response

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

[comius]

cc @pzembrod for triage
cc @oquenchil for potential insights

[oquenchil]

Can you post the error?

[brians-neptune]

ERROR: /home/brian/Desktop/bazel-cc_shared_library-transitive-deps/BUILD:24:10: Linking main_dynamic failed: (Exit 1): cc_wrapper.sh failed: error executing CppLink command (from target //:main_dynamic) external/llvm_toolchain/bin/cc_wrapper.sh @bazel-out/k8-fastbuild/bin/main_dynamic-2.params

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
ld.lld: error: undefined symbol: transitive_return42()
>>> referenced by main.cc
>>>               bazel-out/k8-fastbuild/bin/_objs/main_dynamic/main.pic.o:(dep_return42())
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Use --verbose_failures to see the command lines of failed build steps.

[peakschris]

We are also seeing this issue on linux.

However, windows does not have this issue. On windows, all transitive dependencies to shared libraries are linked. I have validated this with the example repo, modified slightly to support a windows build. I can share these windows fixups if helpful.

This difference in behaviour is quite problematic for us, as a target that builds on windows may be missing a significant number of dependencies in its build file to work on Linux.

I would be in favour of a single approach that works consistently on all platforms.

[oquenchil]

Looking at the history of this change, it actually linked every transitive dependency in the earlier implementation but there were several complaints about that. In other words, linking everything as suggested here would not be the right behavior for others.

Can you refactor your cc_shared_library graph? Would it be possible to merge this cc_library to be part of transitive?

If that's not case and there are real world cases where refactoring the BUILD files is not possible, what I can suggest is that cc_shared_library gets an "exports" attribute where one would list other cc_shared_libraries that must be linked to the dependent every time.

[peakschris]

@oquenchil can you tell from the code why Windows and Linux behaviour is different? Is it a windows bug that dlls are linked transitively? If so, I will raise a new issue with reproducer.

[oquenchil]

It's a requirement on Windows to link the transitive shared libaries. That's working as intended.

[brians-neptune]

Looking at the history of this change, it actually linked every transitive dependency in the earlier implementation but there were several complaints about that. In other words, linking everything as suggested here would not be the right behavior for others.

Do you remember what the complaints for linking all the transitively-depended-upon shared libraries were @oquenchil ? Doing so would not change the full set of shared libraries needed by the target (aka those shown by ldd). It only makes the shared library linking graph "denser", which I don't see any downsides to.

Can you refactor your cc_shared_library graph? Would it be possible to merge this cc_library to be part of transitive?

For my use case, merging or otherwise refactoring the cc_librarys is not an option. There are many shared libraries which are individually dlopened and cannot be combined (they need to have the correct symbols in them). These shared libraries are not linked with cc_shared_library because they need better control over that linking, but they have many common dependencies which need to be split out into appropriate shared libraries so they can all be loaded into a single process without creating multiple definitions. cc_shared_library almost solves this, except for this bug which means I ended up adding lots and lots of redundant dynamic_deps on the top-level binaries, which is not maintainable.

Specifically, I've got a fork of https://github.com/mvukov/rules_ros2 where I did this.

If that's not case and there are real world cases where refactoring the BUILD files is not possible, what I can suggest is that cc_shared_library gets an "exports" attribute where one would list other cc_shared_libraries that must be linked to the dependent every time.

I question the use case for ever having C++ dependencies which aren't exported. C++ uses inline semantics for many things (both explicitly and implicitly, ie definitions in class bodies and constexpr among others, there are a surprising number of esoteric implicit ones), where the compiler gets to choose whether to inline something or link against it. It seems like a footgun to have these semantics where code will work sometimes, but then the compiler changes its mind (or somebody uses a different toolchain) and it breaks.

This doesn't apply as much to C because C doesn't use those kind of "sometimes-linked" definitions as much, but C does still have them.