[PATCH] D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections.

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 30 03:26:06 PDT 2018


jhenderson added inline comments.


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test:7-8
+# RUN: llvm-objdump -s %t.o -section=.debug_foo | FileCheck %s
+# RUN: llvm-objdump -s %t-compressed.o | FileCheck %s --check-prefix=CHECK-COMPRESSED
+# RUN: llvm-readobj -s %t-compressed.o | FileCheck %s --check-prefix=CHECK-FLAGS
+
----------------
jhenderson wrote:
> Is there a particular reason you aren't doing the two checks in the same run of FileCheck?
Make that four... I think you should be able to simplify the number of runs down by using multiple switches in the same command-line (e.g. -relocations and -s in llvm-readobj).


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test:13
+
+# CHECK-RELOCATION: Relocations [
+# CHECK-RELOCATION-NEXT:   .rela.debug_foo {
----------------
Could you also check the Info field, please, to show which section you now refer to.


================
Comment at: tools/llvm-objcopy/Object.h:17
 #include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/StringTableBuilder.h"
----------------
jhenderson wrote:
> plotfi wrote:
> > jhenderson wrote:
> > > You can get rid of this from the header by forward-declaring the DebugCompressionType enum.
> > You can not. Usage of DebugCompressionType::GNU etc makes this not possible unless you want to copy the enum class (which I would like not to).
> You aren't using them in the header file, as far as I can see. Move the include into the .cpp if it is needed there. Same goes for Compression.h.
You should still forward declare the enum in the header and include MCTargetOptions.h in the cpp file.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:426
+
+    for (auto &Sec : Obj.sections()) {
+      RelocationSection *R = dyn_cast<RelocationSection>(&Sec);
----------------
This is a bad way to do this. It means you'll iterate over the whole set of sections once for each compressed section again, just looking for a specific relocation section that may not even exist.

I think the better way to do it would be to make your vector a map, mapping relocation sections to their debug section, and then it can be done in the same loop as above, where you are already iterating over all sections. Something like this partial pseudo-code:

```
Map<SectionBase *, SmallVector<SectionBase*, 1>> ToCompress;
for (auto &Sec : Obj.sections()) {
  if (shouldCompress(Sec))
    ToCompress.insert(Sec);
  else if (auto RelocSec = dyn_cast<RelocationSection>(Sec)) {
    auto TargetSection = RelocSec->getSection();
    if (shouldCompress(TargetSection))
      ToCompress[TargetSection].push_back(RelocSection);
  }
}
```
Two things to watch out for: 1) it is technically legal to have multiple relocation sections targeting the same section, so it needs to be a vector not a single section. 2) It is possible for the relocation section to be before or after the target section in the section list.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:427
+    for (auto &Sec : Obj.sections()) {
+      RelocationSection *R = dyn_cast<RelocationSection>(&Sec);
+      if (R && R->getSection() == S)
----------------
This is one case where you can use auto, because the type is obvious from the same line of code.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:432
+
+    RemovePred = [RemovePred, S](const SectionBase &Sec) {
+      return &Sec == S || RemovePred(Sec);
----------------
jakehehrlich wrote:
> I think we can do this more efficiently now that we don't remove only if space is actually improved.
I'm guessing you mean this should be simply `return (isDebugSection(Sec) && Sec.Name != ".gdb_index") || RemovePred(Sec)` and moved out of the loop. That's what I'd do.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678





More information about the llvm-commits mailing list