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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 53 inline comments as done.
plotfi 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:
> 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).
Different tools?


================
Comment at: tools/llvm-objcopy/Object.cpp:162
+    Chdr->ch_size = Sec.DecompressedSize;
+    Chdr->ch_addralign = Sec.Align;
+    Buf += sizeof(*Chdr);
----------------
jakehehrlich wrote:
> You can make the alignment of this section smaller by storing the alignment of the uncompressed section as a separate field and then just using an alignment of 8 for Sec.Align in the constructor. That will help decrease the size of most fields and avoid unaligned reads/writes for Chdr for sections that have less than 8 byte alignment.
> 
> For instance if a section had page alignment (I can't imagine that happening) you'd save up to almost a page of space by setting the alignment smaller. For debug sections I don't think it will save anything or generally even avoid unaligned reads/writes but it's my preference either way.
So Chdr->ch_addralign = original alignment
Sec.Align = 8 

??


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:426
+
+    for (auto &Sec : Obj.sections()) {
+      RelocationSection *R = dyn_cast<RelocationSection>(&Sec);
----------------
jhenderson wrote:
> 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.
For now I've added a more simple solution. In the top-level loop I collect the relocation sections that point to compresable debug sections and then in the next loop's inner loop I check for the relocation.


Repository:
  rL LLVM

https://reviews.llvm.org/D49678