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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

How do you expect -gsplit-dwarf to interact with the other two options? In other words, if a user specifies it with other or both of the other options, what do you expect the result to be?

I ask because I think the behaviour might be a little unexpected in some combinations of these three switches.



================
Comment at: test/tools/llvm-objcopy/drawf-fission.test:8
+# RUN: diff %t2 %t4
+
+#DWARF:     Name: .debug_loc.dwo
----------------
I think you've gone too far in stripping things down, because now there's no way of knowing that only the expected sections are in the output. You also need to check the number of sections as well (i.e. the corresponding elf header field). This test might still pass if objcopy were to continue to emit the other sections!


================
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"));
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > What is "only-keep-dwo"? 
> I had the option wrong to begin with and this is a remnant of that mistake. I meant "extract-dwo"
Looking at the online documentation, gsplit-dwarf is a compiler switch, rather than an objcopy switch. I want to be sure with this switch name that you have chosen it deliberately to match this, because I'd have probably just called it "split-dwarf" or even "split-dwo" (to match the other two options).

It's also not clear to me what the "gsplit-dwarf file" is. I think you need to reword the help text here.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:66
+cl::opt<bool> StripDwo("strip-dwo",
+                       cl::desc("Remove all DWARF info from file"));
+cl::opt<bool> ExtractDwo("extract-dwo",
----------------
This help text is incorrect - we don't remove all DWARF info, just the DWO sections, leaving the skeleton debug information.


================
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>
----------------
There's a similar issue with this help text - we do remove some DWARF information, but keep the DWO info.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:75
+template <class ELFT>
+bool StripDwoPred(const Object<ELFT> &Obj, const SectionBase &Sec) {
+  // We just simply remove the .dwo sections. That's it! This has the
----------------
Obj is an unneeded argument (and therefore so is the templating, I believe).

Also, I think this function may be better off being called IsDWOSection or similar, since that's all it's doing. It's more general purpose than a predicate for some other function.

Given that, the comment probably needs moving somewhere else.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:83
+template <class ELFT>
+bool OnlyKeepDwoPred(const Object<ELFT> &Obj, const SectionBase &Sec) {
+  // We can't remove the section header string table.
----------------
Dwo should be capitalized for consistency, since it is an abbreviation.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:87
+    return false;
+  // Otherwise we just want to keep what we removed in the other file.
+  return !StripDwoPred(Obj, Sec);
----------------
I think this comment needs rewording, since there may not be another file, if using extract-dwo directly.


Repository:
  rL LLVM

https://reviews.llvm.org/D39207