This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Adjust --strip-dwo behavior
ClosedPublic

Authored by alexander-shaposhnikov on Feb 2 2018, 3:30 PM.

Details

Summary

If the output file is not specified make the modifications in-place (like binutils objcopy does).
This fixes the behavior of clang -gsplit-dwarf (if clang is configured to use llvm-objcopy), previously
it was creating .dwo files but still leaving those sections in the original binary.

Test plan: make check-all

Diff Detail

Repository
rL LLVM

Event Timeline

Test case?

Hmm... I vaguely remember thinking that llvm-objcopy did the work in place already, but on further inspection, it doesn't and I don't think it ever did. The current behaviour is to output the file to a file called "-" if there is no output filename specified. I think this is wrong and not what was intended, but I'd like Jake's feedback on that one. For reference, GNU objcopy does its operations in place if there is no output file, at least for the very limited set that I tried. Given this, I think that the change shouldn't be specific to --strip-dwo, but should always be the case, i.e. the if-statement simply becomes:

if (!OutputFilename.getNumOccurrences())
    Output = InputFilename;
compnerd requested changes to this revision.Feb 5 2018, 5:10 PM

@jhenderson I believe that you are correct with the analysis of the behavior of binutils' objcopy. Definitely should have a test case for this.

This revision now requires changes to proceed.Feb 5 2018, 5:10 PM

Yeah outputting to "-" was a mistake I made in literally the first patch that got this all started. In fact I inherited some starter code (file reading, error handeling, etc..) from Petr and that was from that. I believe Petr ripped it out of some other tool like yaml2obj (which outputs to standard out). This bug is probably the oldest remaining bug in the codebase. I feel a strange attachment to it now even though I just learned about it.

I agree with James, this should not be specific to StripDWO.

tools/llvm-objcopy/llvm-objcopy.cpp
75

Can you make this decleration the following instead?

static cl::opt<std::string> OutputFilename(cl::Positional, cl::desc("[ <output> ] "));
alexander-shaposhnikov marked an inline comment as done.
alexander-shaposhnikov edited the summary of this revision. (Show Details)Feb 9 2018, 3:27 PM
jakehehrlich accepted this revision.Feb 9 2018, 3:27 PM

One nit and then this LGTM

tools/llvm-objcopy/llvm-objcopy.cpp
76

nit: Could you also add the "[ ... ]" brackets so that the help message shows that the output is optional. other than that this LGTM

alexander-shaposhnikov marked an inline comment as done.Feb 9 2018, 3:30 PM
compnerd accepted this revision.Feb 9 2018, 3:32 PM
compnerd added inline comments.
test/tools/llvm-objcopy/strip-dwo-inplace.test
6 ↗(On Diff #133701)

Might be better to write this test as:

CHECK: SectionHeaderCount: 24
CHECK-NOT: Name: [[.*]]dwo
This revision is now accepted and ready to land.Feb 9 2018, 3:32 PM
This revision was automatically updated to reflect the committed changes.
jakehehrlich added inline comments.Feb 9 2018, 3:37 PM
test/tools/llvm-objcopy/strip-dwo-inplace.test
6 ↗(On Diff #133701)

I actually prefer this as it verifies a) that all dwo sections were removed and that b) no sections that weren't supposed to be removed were removed. Where as your proposed test only verifies that the dwo sections were removed.