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

[alexander-shaposhnikov]

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

[jhenderson]

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]

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

[jakehehrlich]

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 ↗

(On Diff #132696)

Can you make this decleration the following instead?

static cl::opt<std::string> OutputFilename(cl::Positional, cl::desc("[ <output> ] "));

[jakehehrlich]

One nit and then this LGTM

tools/llvm-objcopy/llvm-objcopy.cpp

75 ↗

(On Diff #133700)

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]

Address nit.

[compnerd]

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

[jakehehrlich]

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.

llvm/trunk/test/tools/llvm-objcopy/strip-dwo-inplace.test

+# RUN: cp %p/Inputs/dwarf.dwo %t
+# RUN: llvm-objcopy -strip-dwo %t
+# RUN: llvm-readobj -file-headers -sections %t | FileCheck %s
+
+CHECK:     SectionHeaderCount: 24
+
+CHECK:    Name: .text
+CHECK:    Name: .rodata.str1.1
+CHECK:    Name: .debug_str
+CHECK:    Name: .debug_abbrev
+CHECK:    Name: .debug_info
+CHECK:    Name: .debug_ranges
+CHECK:    Name: .debug_macinfo
+CHECK:    Name: .debug_addr
+CHECK:    Name: .debug_pubnames
+CHECK:    Name: .debug_pubtypes
+CHECK:    Name: .comment
+CHECK:    Name: .note.GNU-stack
+CHECK:    Name: .debug_frame
+CHECK:    Name: .debug_line
+CHECK:    Name: .symtab
+CHECK:    Name: .rela.text
+CHECK:    Name: .rela.debug_info
+CHECK:    Name: .rela.debug_addr
+CHECK:    Name: .rela.debug_pubnames
+CHECK:    Name: .rela.debug_pubtypes
+CHECK:    Name: .rela.debug_frame
+CHECK:    Name: .rela.debug_line
+CHECK:    Name: .strtab

llvm/trunk/tools/llvm-objcopy/llvm-objcopy.cpp

LLVM_ATTRIBUTE_NORETURN void reportError(StringRef File, Error E) {
   OS.flush();
   errs() << ToolName << ": '" << File << "': " << Buf;
   exit(1);
 }
 
 } // end namespace llvm
 
 static cl::opt<std::string> InputFilename(cl::Positional, cl::desc("<input>"));
-static cl::opt<std::string> OutputFilename(cl::Positional, cl::desc("<output>"),
-                                           cl::init("-"));
+static cl::opt<std::string> OutputFilename(cl::Positional, cl::desc("[ <output> ]"));
+
 static cl::opt<std::string>
     OutputFormat("O", cl::desc("Set output format to one of the following:"
                                "\n\tbinary"));
 static cl::list<std::string> ToRemove("remove-section",
                                       cl::desc("Remove <section>"),
                                       cl::value_desc("section"));
 static cl::alias ToRemoveA("R", cl::desc("Alias for remove-section"),
                            cl::aliasopt(ToRemove));
int main(int argc, char **argv) {
   ToolName = argv[0];
   if (InputFilename.empty()) {
     cl::PrintHelpMessage();
     return 2;
   }
 
   auto Reader = CreateReader();
   auto Obj = Reader->create();
-  auto Writer = CreateWriter(*Obj, OutputFilename);
+  StringRef Output =
+      OutputFilename.getNumOccurrences() ? OutputFilename : InputFilename;
+  auto Writer = CreateWriter(*Obj, Output);
   HandleArgs(*Obj, *Reader);
   Writer->finalize();
   Writer->write();
 }