Page MenuHomePhabricator

Make -gsplit-dwarf generally available
Needs ReviewPublic

Authored by trixirt on May 11 2018, 7:00 PM.

Details

Summary

Change CLANG_DEFAULT_OBJCOPY from objcopy to llvm-objcopy
Remove is-linux checks
Add dwarf splitting to FreeBSD's assembler job.

Diff Detail

Event Timeline

trixirt created this revision.May 11 2018, 7:00 PM
jakehehrlich added inline comments.May 14 2018, 10:48 AM
lib/Driver/ToolChains/Clang.cpp
4818–4819

Maybe re-add a TODO to point out that *some* objcopy is needed be it GNU objcopy or llvm-objcopy? Ideally this would be done by the compiler and not by an external tool.

test/Driver/split-debug.c
7

I think I may have missed something here. I thought this would change all of clang's objcopy invocations regardless of the target triple. What chooses "objcopy" if the target system is linux or linux-gnu? I think this might be passing because that line still matches "llvm-objcopy ..."

14

When I wrote this I wanted this test to pass regardless of which objcopy was used so I just left it as "objcopy" because that would still pass if "llvm-objcopy" or "objcopy" was used. For instance my team builds the toolchain and we consider a build a fail of all tests don't pass. If someone wanted to use a GNU objcopy and built and tested toolchains the same way they would run into this issue.

I'm generally ok with this but I think some important checks need to occur first. Namely I think I'd like to check that Chromium still builds with this option since I'd imagine they'd be affected by this.

trixirt updated this revision to Diff 146970.May 15 2018, 5:37 PM

Improve comment in CLANG_DEFAULT_OBJCOPY to specify what is required of objcopy.
Remove the llvm- prefix from testing to allow for override of default value.

FWIW Peter has some patches to move object emission away from objcopy that I'm on the hook to review here shortly so the objcopy part of this should become unnecessary and can just have us able to emit dwarf5 compatible split dwarf on any platform.

-eric

Yeah I haven't heard back from Lexan nor have I tried reproducing such a build myself and if we're that close to not relaying on this sort of hack I'd much rather wait on this than risk breaking someone for the sake of a stopgap for such a short period of time. @echristo Can you link the review to that?

pcc added a comment.May 21 2018, 3:24 PM

There were a bunch of them but the last one was D47093 which has already landed :)

Probably all that needs to happen on this change is to replace the isLinux() check with isELF().

In D46791#1107168, @pcc wrote:

There were a bunch of them but the last one was D47093 which has already landed :)

Probably all that needs to happen on this change is to replace the isLinux() check with isELF().

Be good if it didn't even require the isELF check anymore, but that probably requires testing on a few platforms. :)

compnerd added inline comments.May 29 2018, 3:57 PM
test/Driver/split-debug.c
23

NIT: macOS or the legacy spelling of Mac OS X.

aprantl added inline comments.Sep 3 2019, 1:11 PM
test/Driver/split-debug.c
28

split dwarf does not make sense on macOS since the debug info is never linked into the binary to begin with, so you might as well not test this configuration, or even better error out when it is chosen.

This is obsolete I think. Clang doesn't use llvm-objcopy for this anymore if I understand correct. @pcc should be able to confirm.