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

incompatible_do_not_split_linking_cmdline: Order of C++ link flags affected #7687

Open
oquenchil opened this issue Mar 11, 2019 · 5 comments

Comments

Projects
None yet
3 participants
@oquenchil
Copy link
Contributor

commented Mar 11, 2019

Flag: --incompatible_do_not_split_linking_cmdline
Available since: 0.25 (March 2019 release)
Will be flipped in: 0.26 (April 2019 release)
Tracking issue: #7670

This changes how flags are passed to the linker. If you don't have your own custom CROSSTOOL, then you don't need to migrate anything, but you should still test with this flag to see that nothing breaks. If it does, please let us know.

If you have your own custom CROSSTOOL:
If the CROSSTOOL doesn't support linker parameter files, then you don't have to migrate anything. If it supports linker parameter file but you do not specify the feature 'linker_param_file' in your CROSSTOOL, then you don't have to migrate anything.

If you specify the feature 'linker_param_file' and the linker is using gcc-style linker flags, then you should modify the feature so that instead of having a parameter file specified like this:
flag: '-Wl,@%{linker_param_file}'
it is like this:
flag: '@%{linker_param_file}'

In other words, the parameter file should not be passed directly to the linker with -Wl,. In addition to this you should specify the feature 'do_not_split_linking_cmdline' in your CROSSTOOL, whether it's enabled or disabled it does not matter, we only check its presence. This will tell Bazel to use the new behavior with this toolchain. After the flip of incompatible_disable_legacy_cc_provider this feature will do nothing, it can safely be deleted.

This change will put every flag inside the parameter file whereas before only flags starting with -l were put there, i.e.: all libraries. This might cause the order of link flags to change with respect to what you originally had in your CROSSTOOL, if it does you will have to re-order the corresponding flags. If you have to make any changes to the CROSSTOOL, make sure that you add the feature 'do_not_split_linking_cmdline' too. Example of change:

Old:

feature {
  name: 'linker_param_file',
  flag_set {
         action: 'c++-link-executable'
         action: 'c++-link-dynamic-library'
         action: 'c++-link-nodeps-dynamic-library'
         flag_group {
             expand_if_all_available: 'linker_param_file'
             flag: '-Wl,@%{linker_param_file}'
         }
}

New:

feature {
  name: 'do_not_split_linking_cmdline'
}
feature {
  name: 'linker_param_file',
  flag_set {
         action: 'c++-link-executable'
         action: 'c++-link-dynamic-library'
         action: 'c++-link-nodeps-dynamic-library'
         flag_group {
             expand_if_all_available: 'linker_param_file'
             flag: '@%{linker_param_file}'
         }
}
@dkelmer

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

@oquenchil, there don't seem to be associated PRs submitted. Does the flag exist already? If yes, by when do you want people to migrate? If the flag exists, it will be available in the currently being cut release, 0.25. The earliest you can require people to migrate by is the 0.26 release. Is that the plan?

@dkelmer

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

@oquenchil Seems like the flag does exist and that it was flipped (http://google3/third_party/bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java?l=836&rcl=241059021). I think that the flag flip needs to be rolled back because there wasn't a migration window.

@hlopko

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

You're looking into the wrong flag :) This is the change that introduced this flag: 3010e57. I'm changing the labels accordingly.

@dkelmer

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Oh, the title of the issue is inconsistent with the beginning of the content of the issue which is why I got confused :)
Thanks for updating the labels!

@hlopko

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

Updated, sorry for the confusion.

bazel-io pushed a commit that referenced this issue May 14, 2019

C++: Make it unnecessary to patch Linux crosstool in Bazel
Tested with Bazel CI downstream projects. These fail for unrelated reasons. Both together show that everything is green with my change:
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/971
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/969

This change makes it unnecessary to patch most CppActionConfig features only in Linux for now.

The build_interface_libraries feature is still patched. Once Bazel is released with this change: 4eb7683
we can stop patching it.

Also a separate change should do the same for Windows and Mac.

This also unblocks flipping --incompatible_do_not_split_linking_cmdline

#8303, #6926, #7687

RELNOTES:none
PiperOrigin-RevId: 248117783
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.