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

Include the symlinked dwp/objcopy/strip files in the toolchain #108

Merged
merged 6 commits into from
Sep 28, 2021

Conversation

fishcakez
Copy link
Contributor

Without the :llvm target the toolchain files are missing when using sandbox.

@rrbutani rrbutani self-assigned this Sep 27, 2021
@rrbutani
Copy link
Collaborator

Thanks for the PR and good catch.

Do you have an example of an action that fails without this fix? I'd like to add a test to ensure that we don't make this mistake again but I'm struggling to come up something that exposes this issue; do we need --features=per_object_debug_info or something?

@fishcakez
Copy link
Contributor Author

fishcakez commented Sep 27, 2021

--fission=yes
--features=per_object_debug_info

Then try to build a cc_binary with .dwp appended (must be explicit). If no .dwo files are generated it wont fail because dwp wont be called and an empty .dwp file is created.

@fishcakez
Copy link
Contributor Author

@rrbutani this worked on our build nicely, except we need a further patch of very similar nature. I hope you don't mind if i add strip file groups to this PR.

@fishcakez fishcakez changed the title Fix including symlink extra files in dwp/objcopy Fix including symlink extra files in dwp/objcopy/strip Sep 27, 2021
@fishcakez
Copy link
Contributor Author

To reproduce for stripping try to build a cc_binary with .stripped appended (must be explicit).

@rrbutani
Copy link
Collaborator

@rrbutani this worked on our build nicely, except we need a further patch of very similar nature. I hope you don't mind if i add strip file groups to this PR.

Yup, that's absolutely fine; they're the same underlying issue.

@rrbutani
Copy link
Collaborator

Sorry for the delay; it took me a while to realize that the reason I couldn't reproduce this locally (clang refused to produce .dwo files even with your patch) is because clang understandably does nothing (doesn't even emit empty .dwo files) with -gsplit-dwarf when targeting macOS and compiling object files.

I also got a bit sidetracked trying to figure out why --features=per_object_debug_info is needed in addition to --fission (#109) but none of that has much to do with this PR.


I'll add a build test for a cc_binary that checks that the stripped target builds as well as a test for the .dwp target that has the per_object_debug_info feature enabled and is gated to not run on macOS targets (I'll just enable fission globally in the tests).

After that this PR should be good to go.

This has some things we'll eventually want to remove; see bazel-contrib#109 for some context.
@siddharthab
Copy link
Contributor

Thank you @rrbutani for taking care of this.

@rrbutani
Copy link
Collaborator

Thank you @rrbutani for taking care of this.

No problem!


I believe the failures in CI we're seeing now are because of this change in LLVM 12 (and also GCC 11) that no longer has -gsplit-dwarf implicitly mean -g2. Without this, in -c fastbuild (the default) the -g0 that's added to the compile commands isn't overridden and thus no .dwo files are produced.

bazelbuild/bazel#14038 tracks this and bazelbuild/rules_cc#115 has a fix that will fix the problem on our end, if merged.

For now, to work around this without downgrading our LLVM version or switching the tests to use -c dbg or special copts, I'll have the transition also set the compilation mode to dbg and I'll update #109 to say that we should use the patch in bazelbuild/rules_cc#115 if/when we switch to using our own version of unix_cc_toolchain_config.bzl.

…ated with clang-12+

Issue bazel-contrib#109 and [this comment](bazel-contrib#108 (comment))
have some context; the short version is that `-gsplit-dwarf` no longer implies `-g2` which means under
`-c fastbuild` (implies `-g0`) no `.dwo` files are created.

There's a patch to `unix_cc_toolchain_config.bzl` but for now we'll just use `-c dbg` for this one
target using a transition.
@rrbutani
Copy link
Collaborator

@siddharthab does this PR look okay?

Also (assuming this is fine to merge) is it okay if I update the README and tag 0.6.3 after merging it?

@rrbutani rrbutani changed the title Fix including symlink extra files in dwp/objcopy/strip Include the symlinked dwp/objcopy/strip files in the toolchain Sep 28, 2021
@rrbutani rrbutani merged commit 806346a into bazel-contrib:master Sep 28, 2021
@siddharthab
Copy link
Contributor

@rrbutani Yes, please go ahead. If there are no other changes in the works for the immediate future, you can push a new tag.

@fishcakez fishcakez deleted the fix-dwp branch September 28, 2021 17:20
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

Successfully merging this pull request may close these issues.

3 participants