Make linking the same shared library twice a rule error by agluszak · Pull Request #11721 · bazelbuild/bazel

agluszak · 2020-07-07T11:40:23Z

As more and more people start using the power of Starlark transitions, we are seeing more people bumping into a precondition baked deep in the C++ rules implementation which checks that a shared library being linked is in the same configuration directory. The check is there to make sure that the same library built in different configurations is not linked more than once.
Aside from the fact that it should be a proper rule error and not a crash, people are hitting this check when writing cc_import targets that have a transition applied to them. There are other ways to guarantee that the same library is not linked more than once without completely restricting the ability to link a library built in a different configuration.
@oquenchil

This PR makes linking the same shared library twice a rule error instead of a crash and it makes it possible to link a library built in a different configuration. A map from library identifiers to library paths is introduced, making sure that each shared library is linked at most once. Only dynamic linking is taken into account, for static linking see #11727

Fixes #11167 (the check which was causing the NPE was not directly related to transitions, but because it was removed, that NPE should no longer happen)

oquenchil

Can you add a description and also add "Fixes #11167" to it?

But before you add that it fixes, please verify that it indeed fixes it. In #11167 they have a link to a PR which reproduces the problem and they also have the necessary command.

Try the command with released bazel (should crash) and try with your custom built binary that has this change (should succeed)

agluszak · 2020-07-13T12:47:44Z

But before you add that it fixes, please verify that it indeed fixes it. In #11167 they have a link to a PR which reproduces the problem and they also have the necessary command.

Try the command with released bazel (should crash) and try with your custom built binary that has this change (should succeed)

With released Bazel it crashed with an NPE, with custom built binary it fails with

ERROR: /usr/local/google/home/agluszak/code/rules_go/tests/core/c_linkmodes/BUILD.bazel:52:8: C++ compilation of rule '//tests/core/c_linkmodes:c-shared_test' failed (Exit 1): gcc failed: error executing command 
  (cd /usr/local/google/home/agluszak/.cache/bazel/_bazel_agluszak/f95e0ba868b0609c6e710465f999aad9/sandbox/linux-sandbox/38/execroot/io_bazel_rules_go && \
  exec env - \
    LD_LIBRARY_PATH=/usr/lib/mesa-diverted/x86_64-linux-gnu:/usr/lib/x86_64-linux-gnu/mesa:/usr/lib/x86_64-linux-gnu/dri:/usr/lib/x86_64-linux-gnu/gallium-pipe \
    PATH=/usr/local/google/home/agluszak/bin:/usr/lib/google-golang/bin:/usr/local/buildtools/java/jdk/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin \
    PWD=/proc/self/cwd \
  /usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer -MD -MF bazel-out/k8-fastbuild/bin/tests/core/c_linkmodes/_objs/c-shared_test/add_test_shared.pic.d '-frandom-seed=bazel-out/k8-fastbuild/bin/tests/core/c_linkmodes/_objs/c-shared_test/add_test_shared.pic.o' -fPIC -iquote . -iquote bazel-out/k8-fastbuild/bin -iquote external/bazel_tools -iquote bazel-out/k8-fastbuild/bin/external/bazel_tools -Ibazel-out/k8-fastbuild-ST-1e89748556c3/bin -fno-canonical-system-headers -Wno-builtin-macro-redefined '-D__DATE__="redacted"' '-D__TIMESTAMP__="redacted"' '-D__TIME__="redacted"' -c tests/core/c_linkmodes/add_test_shared.c -o bazel-out/k8-fastbuild/bin/tests/core/c_linkmodes/_objs/c-shared_test/add_test_shared.pic.o)
Execution platform: @local_config_platform//:host

Use --sandbox_debug to see verbose messages from the sandbox gcc failed: error executing command 
  (cd /usr/local/google/home/agluszak/.cache/bazel/_bazel_agluszak/f95e0ba868b0609c6e710465f999aad9/sandbox/linux-sandbox/38/execroot/io_bazel_rules_go && \
  exec env - \
    LD_LIBRARY_PATH=/usr/lib/mesa-diverted/x86_64-linux-gnu:/usr/lib/x86_64-linux-gnu/mesa:/usr/lib/x86_64-linux-gnu/dri:/usr/lib/x86_64-linux-gnu/gallium-pipe \
    PATH=/usr/local/google/home/agluszak/bin:/usr/lib/google-golang/bin:/usr/local/buildtools/java/jdk/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin \
    PWD=/proc/self/cwd \
  /usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer -MD -MF bazel-out/k8-fastbuild/bin/tests/core/c_linkmodes/_objs/c-shared_test/add_test_shared.pic.d '-frandom-seed=bazel-out/k8-fastbuild/bin/tests/core/c_linkmodes/_objs/c-shared_test/add_test_shared.pic.o' -fPIC -iquote . -iquote bazel-out/k8-fastbuild/bin -iquote external/bazel_tools -iquote bazel-out/k8-fastbuild/bin/external/bazel_tools -Ibazel-out/k8-fastbuild-ST-1e89748556c3/bin -fno-canonical-system-headers -Wno-builtin-macro-redefined '-D__DATE__="redacted"' '-D__TIMESTAMP__="redacted"' '-D__TIME__="redacted"' -c tests/core/c_linkmodes/add_test_shared.c -o bazel-out/k8-fastbuild/bin/tests/core/c_linkmodes/_objs/c-shared_test/add_test_shared.pic.o)
Execution platform: @local_config_platform//:host

Use --sandbox_debug to see verbose messages from the sandbox
tests/core/c_linkmodes/add_test_shared.c:2:10: fatal error: tests/core/c_linkmodes/adder_shared.h: No such file or directory
    2 | #include "tests/core/c_linkmodes/adder_shared.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
Target //tests/core/c_linkmodes:c-shared_test failed to build

...but there's no such file indeed

…crash

googlebot · 2020-07-14T08:57:11Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-07-14T09:08:52Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

…ts linked in correctly. Add more details to the rule error.

…hared-lib-twice-after-transition # Conflicts: # src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java

oquenchil

LGTM

@oquenchil

> As more and more people start using the power of Starlark transitions, we are seeing more people bumping into a precondition baked deep in the C++ rules implementation which checks that a shared library being linked is in the same configuration directory. The check is there to make sure that the same library built in different configurations is not linked more than once. > Aside from the fact that it should be a proper rule error and not a crash, people are hitting this check when writing cc_import targets that have a transition applied to them. There are other ways to guarantee that the same library is not linked more than once without completely restricting the ability to link a library built in a different configuration. @oquenchil This PR makes linking the same shared library twice a rule error instead of a crash and it makes it possible to link a library built in a different configuration. A map from library identifiers to library paths is introduced, making sure that each shared library is linked at most once. Only dynamic linking is taken into account, for static linking see bazelbuild#11727 Fixes bazelbuild#11167 (the check which was causing the NPE was not directly related to transitions, but because it was removed, that NPE should no longer happen) Closes bazelbuild#11721. PiperOrigin-RevId: 322563028

agluszak requested review from lberki and oquenchil as code owners July 7, 2020 11:40

googlebot added the cla: yes label Jul 7, 2020

lberki removed their request for review July 7, 2020 11:41

agluszak mentioned this pull request Jul 8, 2020

It's possible to statically link the same library twice #11727

Closed

oquenchil requested changes Jul 13, 2020

View reviewed changes

agluszak added 4 commits July 14, 2020 07:55

Make linking the same shared library twice a rule error instead of a …

fafae78

…crash

Implement getLibraryIdentifier() in LinkerInput

99b3fc4

Minor fixes after code review

7514a73

Fix NPE for CppLinkActionBuilder

807a48b

agluszak requested review from ahumesky, aiuto, brandjon, c-parsons, gregestren, jin, laurentlb and vladmos as code owners July 14, 2020 08:56

googlebot added cla: no and removed cla: yes labels Jul 14, 2020

agluszak marked this pull request as draft July 14, 2020 09:00

agluszak force-pushed the linking-shared-lib-twice-after-transition branch from eb489f6 to 807a48b Compare July 14, 2020 09:08

googlebot added cla: yes and removed cla: no labels Jul 14, 2020

agluszak removed request for ahumesky, aiuto and brandjon July 14, 2020 09:11

agluszak removed request for c-parsons, gregestren, jin, laurentlb and vladmos July 14, 2020 09:11

agluszak marked this pull request as ready for review July 14, 2020 09:23

agluszak marked this pull request as draft July 14, 2020 09:23

agluszak added 2 commits July 14, 2020 16:09

Make sure that libDir is not null

17f2fd9

Add a test to make sure that a unique shared lib with a transition ge…

2de984c

…ts linked in correctly. Add more details to the rule error.

agluszak marked this pull request as ready for review July 14, 2020 16:42

agluszak requested a review from oquenchil July 14, 2020 16:43

oquenchil reviewed Jul 20, 2020

View reviewed changes

Comment thread src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java Outdated

agluszak added 3 commits July 20, 2020 14:03

Merge remote-tracking branch 'remotes/upstream/master' into linking-s…

1a7df8f

…hared-lib-twice-after-transition # Conflicts: # src/main/java/com/google/devtools/build/lib/rules/cpp/LibrariesToLinkCollector.java

Minor formatting correction

f648b6c

Make sure the analysis actually runs before the assertion

3089261

oquenchil self-assigned this Jul 21, 2020

oquenchil approved these changes Jul 21, 2020

View reviewed changes

bazel-io closed this in a90c408 Jul 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make linking the same shared library twice a rule error#11721

Make linking the same shared library twice a rule error#11721
agluszak wants to merge 9 commits intobazelbuild:masterfrom
agluszak:linking-shared-lib-twice-after-transition

agluszak commented Jul 7, 2020 •

edited

Loading

Uh oh!

oquenchil left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agluszak commented Jul 13, 2020

Uh oh!

googlebot commented Jul 14, 2020

Uh oh!

googlebot commented Jul 14, 2020

Uh oh!

Uh oh!

oquenchil left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

agluszak commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oquenchil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agluszak commented Jul 13, 2020

Uh oh!

googlebot commented Jul 14, 2020

Uh oh!

googlebot commented Jul 14, 2020

Uh oh!

Uh oh!

oquenchil left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

agluszak commented Jul 7, 2020 •

edited

Loading