[llvm-objcopy] - SImplify `isCompressable` and fix the issue relative.

[grimar]

Summary

When --compress-debug-sections is given, llvm-objcopy do not compress
sections that have "ZLIB" header in data. Normally this signature is used
in zlib-gnu compression format. But if zlib-gnu used then the name of the compressed
section should start from .z* (e.g .zdebug_info). If it does not, then it is not
a zlib-gnu format and section should be treated as a normal uncompressed section.

It is a probably unlikely case that some debug sections start from "ZLIB",
but I think we still want to be correct here and also this patch allows to simplify a logic
slightly.

[jhenderson]

This is for llvm-objcopy, not llvm-objdump as claimed in the header and description, right?

[grimar]

In D58908#1417022, @jhenderson wrote:

This is for llvm-objcopy, not llvm-objdump as claimed in the header and description, right?

Oops. Right.

[grimar]

• Fixed llvm-objdump -> llvm-objcopy in the test case.

[MaskRay]

test/tools/llvm-objcopy/ELF/compress-debug-sections-zlib-header.test

7 ↗

(On Diff #189161)

The description may be a bit verbose. I think we can just say that non-.zdebug_ sections whose data does not start with "ZLIB" should not be considered compressed.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

230 ↗

(On Diff #189161)

isDebugSection(Section) also checks ".zdebug". How about just using Section.Name.startswith(".debug_") ?

[grimar]

• Addressed review comments.

test/tools/llvm-objcopy/ELF/compress-debug-sections-zlib-header.test

7 ↗

(On Diff #189161)

I reduced the comment, thanks!

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

230 ↗

(On Diff #189161)

Actually yes, that would be better here I think. Applied your suggestion.

[jhenderson]

test/tools/llvm-objcopy/ELF/compress-debug-sections-zlib-header.test

10 ↗

(On Diff #189284)

"from" -> "with" here and the line below.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

227–230 ↗

(On Diff #189284)

Now that you've changed this further, this can be even simpler, I think:

return !(Section.Flags & ELF::SHF_COMPRESSED) &&
           StringRef(Section.Name).startswith(".debug");

[grimar]

• Addressed review comments.

tools/llvm-objcopy/ELF/ELFObjcopy.cpp

227–230 ↗

(On Diff #189284)

Yep. Thanks!

[jhenderson]

LGTM.

llvm/trunk/test/tools/llvm-objcopy/ELF/compress-debug-sections-zlib-header.test

+# REQUIRES: zlib
+
+# RUN: yaml2obj %s -o %t1.o
+# RUN: llvm-objcopy --compress-debug-sections %t1.o %t2.o
+# RUN: llvm-readobj -s %t2.o | FileCheck %s
+
+# Non-.zdebug* sections whose data does start with "ZLIB" should
+# not be considered as zlib-gnu compressed sections.
+
+# In this test we have 2 normal sections, one of them starts with "ZLIB",
+# another starts with "ZZZZ". Check we compress both as expected.
+
+# CHECK:        Name: .debug_foo
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_COMPRESSED
+# CHECK-NEXT:   ]
+
+# CHECK:        Name: .debug_bar
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_COMPRESSED
+# CHECK-NEXT:   ]
+
+--- !ELF
+FileHeader:      
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:        
+  - Name:            .debug_foo
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x0000000000000001
+    # "ZLIB"
+    Content:         5A4C49420000000000000000
+  - Name:            .debug_bar
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x0000000000000001
+    # "ZZZZ"
+    Content:         5A5A5A5A0000000000000000

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

if (Sec.Name == SecName) {
       if (Error E = Buf->commit())
         return E;
       return Error::success();
     }
   }
   return createStringError(object_error::parse_failed, "Section not found");
 }
 
-static bool isCompressed(const SectionBase &Section) {
-  const char *Magic = "ZLIB";
-  return StringRef(Section.Name).startswith(".zdebug") ||
-         (Section.OriginalData.size() > strlen(Magic) &&
-          !strncmp(reinterpret_cast<const char *>(Section.OriginalData.data()),
-                   Magic, strlen(Magic))) ||
-         (Section.Flags & ELF::SHF_COMPRESSED);
-}
-
 static bool isCompressable(const SectionBase &Section) {
-  return !isCompressed(Section) && isDebugSection(Section) &&
-         Section.Name != ".gdb_index";
+  return !(Section.Flags & ELF::SHF_COMPRESSED) &&
+         StringRef(Section.Name).startswith(".debug");
 }
 
 static void replaceDebugSections(
     const CopyConfig &Config, Object &Obj, SectionPred &RemovePred,
     function_ref<bool(const SectionBase &)> shouldReplace,
     function_ref<SectionBase *(const SectionBase *)> addSection) {
   SmallVector<SectionBase *, 13> ToReplace;
   SmallVector<RelocationSection *, 13> RelocationSections;