[PATCH] D39207: [llvm-objcopy] Add support for dwarf fission

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've made a number of inline comments, but it's mostly fine-detail. The overall approach looks fine to me.



================
Comment at: test/tools/llvm-objcopy/drawf-fission.test:6
+# RUN: llvm-readobj -sections %t2 | FileCheck %s -check-prefix=STRIP
+# RUN: diff -rs %t %t3 | FileCheck %s -check-prefix=DIFFEXTRACT
+# RUN: diff -rs %t2 %t4 | FileCheck %s -check-prefix=DIFFSTRIP
----------------
I'm not sure you need the -rs here? Also, do you really need to check the output? Doesn't diff exit with a non-zero exit code if the files differ?


================
Comment at: test/tools/llvm-objcopy/drawf-fission.test:9
+
+#DWARF: Sections [
+#DWARF-NEXT:   Section {
----------------
I think explicitly listing all the sections here and their properties is unnecessary here. Probably better would be to a) check the number of sections is as expected, and then simply do something like:
```
#DWARF: Name: (0)
#DWARF: Name: .debug_loc.dwo
#DWARF: Name: .debug_str.dwo
...
```

If you want to check specific properties (e.g. the Size), then just do Name, Size, Name, Size checks without the NEXT, since that will capture the ones that are needed in the right places (checking the number of sections means that you don't accidentally start checking another section's properties).


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:68
+cl::opt<bool> ExtractDwo("extract-dwo",
+                         cl::desc("Remove all non DWARF info from file"));
+cl::opt<std::string>
----------------
"non DWARF" should be hyphenated, I believe: "non-DWARF".


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:71
+    GSplitDwarf("gsplit-dwarf",
+                cl::desc("equivliant to strip-dwo and then only-keep-dwo on "
+                         "the gsplit-dwaf file"));
----------------
What is "only-keep-dwo"? 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:76
+bool StripDwoPred(const Object<ELFT> &Obj, const SectionBase &Sec) {
+  // We just simply remove the .dwo sections. That's it! The has the consequence
+  // of removing relocations for .dwo sections and symbols defined in .dwo
----------------
The has the -> This has the


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:87-90
+  // We can't remove the note section because we need to match the .dwo file
+  // up with the binary.
+  if (Sec.Type == SHT_NOTE)
+    return false;
----------------
Why do we need this? According to the DWARF 5 appendix, there are tags in the debug information that allow matching the two up. It certainly doesn't mention anything about the SHT_NOTE sections. Note that there are many different kinds of SHT_NOTE sections as well, and not just one single one.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:96
+template <class ELFT>
+void WriteObjFile(const Object<ELFT> &Obj, const std::string &File) {
+  std::unique_ptr<FileOutputBuffer> Buffer;
----------------
File should be a StringRef, I believe?

I'd also avoid the unnecessary abbreviation here. Either call the function WriteObject or WriteObjectFile.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:112
+void SplitDWOToFile(const ELFObjectFile<ELFT> &ObjFile,
+                    const std::string &File) {
+  // Construct a second output file
----------------
StringRef


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:113
+                    const std::string &File) {
+  // Construct a second output file
+  ELFObject<ELFT> GSplitObj(ObjFile);
----------------
"... for the DWO sections", maybe? Also full stop.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:166
   Obj->finalize();
-  ErrorOr<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
-      FileOutputBuffer::create(OutputFilename, Obj->totalSize(),
-                               FileOutputBuffer::F_executable);
-  if (BufferOrErr.getError())
-    error("failed to open " + OutputFilename);
-  else
-    Buffer = std::move(*BufferOrErr);
-  std::error_code EC;
-  if (EC)
-    report_fatal_error(EC.message());
-  Obj->write(*Buffer);
-  if (auto EC = Buffer->commit())
-    reportError(OutputFilename, EC);
+  WriteObjFile<ELF64LE>(*Obj, OutputFilename.getValue());
 }
----------------
Do you need the explicit template argument list here? Can't it derive it from (*Obj)'s type?


Repository:
  rL LLVM

https://reviews.llvm.org/D39207