[llvm-mc] - Teach llvm-mc to generate compressed debug sections in zlib style.

[grimar]

Summary

Before this patch llvm-mc generated zlib-gnu styled sections.
That means no SHF_COMPRESSED flag was set, magic 'zlib' signature
was used in combination with full size field. Sections were renamed to "*.z*".
This patch reimplements the compression style to zlib one as zlib-gnu looks
to be depricated everywhere atm.

[davide]

lib/MC/ELFObjectWriter.cpp

576 ↗

(On Diff #57486)

Why do you need to change this?

lib/MC/MCSectionELF.cpp

164 ↗

(On Diff #57486)

Why did you change from getType() to Flags. Also, why can't you use the getter as they were used before?

[grimar]

lib/MC/MCSectionELF.cpp

164 ↗

(On Diff #57486)

I made Flags public because I need to add SHF_COMPRESSED flag to it.
In lld if we use the variable directly like I did, we do not using getters setters anymore, because there is no sense.
getters/setters are only useful if can protect something, but if you need and use both of them, then probably you do not need any of in fact. So public variable is better here.
I am not sure about style in any other project that lld, I had no real expirience in changing anything aside yet, but I guess it should be the same ?

[• rafael]

include/llvm/MC/MCContext.h

388 ↗

(On Diff #57486)

So nice to see this go :-)

lib/MC/ELFObjectWriter.cpp

1027 ↗

(On Diff #57486)

So, the size is always written big endian? That is a bit odd, but if that is the case, can you use support::endian::Writer<support::big> instead of the if?

[• rafael]

include/llvm/MC/MCSectionELF.h

65 ↗

(On Diff #57486)

You can delete this one now.

[grimar]

lib/MC/MCSectionELF.cpp

164 ↗

(On Diff #57486)

ah, I changed getType, but I was thinking I am changing getFlag(). Thank you, I'll revisit this one !

[grimar]

• Addressed review comments.

include/llvm/MC/MCContext.h

388 ↗

(On Diff #57486)

I fisrt time saw that code, but the fact of renaming something after giving it a name looked like a hack for me, I was also happy to do that :)
(so zlib-gnu style of compressed sections itself looks like a hack in ELF world for me now, it is good that we are able just to get rid of that instead of continue supporting, I think).

include/llvm/MC/MCSectionELF.h

65 ↗

(On Diff #57486)

Done.

lib/MC/ELFObjectWriter.cpp

576 ↗

(On Diff #57486)

Because Flags is a public member since now, please see below comment.

1027 ↗

(On Diff #57486)

wow, thanks for noticing. At fact the big endian should be used always for zlib-gnu style headers. But for zlib ones (this patch) elf file encoding should be used. I was sure that rule about big endian applies for both, even was not able to imaging a trap here. But there is a difference between styles, so for zlib style - target elf file endianess is a must, I updated the code.

lib/MC/MCSectionELF.cpp

164 ↗

(On Diff #57486)

So i reverted this, it was a mistake change.

[grimar]

I`ll update this one again soon. Forgot to change a piece of code :(

[grimar]

• Updated patch, to write the forgotten type field.
• Also use header variables instead of c++ types (NFC, but looks better IMO).

[davide]

lib/MC/ELFObjectWriter.cpp

576 ↗

(On Diff #57614)

You can change that separately, if you really like. It's unrelated, and makes the patch slightly less readable.

lib/MC/MCSectionELF.cpp

162 ↗

(On Diff #57614)

Unrelated.

[grimar]

lib/MC/ELFObjectWriter.cpp

576 ↗

(On Diff #57614)

Please see my comment below.

lib/MC/MCSectionELF.cpp

162 ↗

(On Diff #57614)

I am sorry but I find this and above change directly related since that changes are connected to what this patch do.
But if you think i need to make Flags variable public at first in separate commit, then I have no problems with that, I just think if this whole patch be approved, then I`ll do 2 commits, but otherwise there is probably no need to perform changes with Flags member.

[davide]

lib/MC/ELFObjectWriter.cpp

576 ↗

(On Diff #57614)

I can review them at the same time but they should be committed separately.

1024 ↗

(On Diff #57614)

static_cast<> instead of C-style cast?

[davide]

lib/MC/ELFObjectWriter.cpp

576 ↗

(On Diff #57614)

I saw your comment. There's no need to change at the same time the access control and remove the getter. Leave it private, and leave the getter to keep the patch small. You can do that in a separate commit.

[grimar]

lib/MC/ELFObjectWriter.cpp

576 ↗

(On Diff #57614)

Sorry but that means I need to intoduce setter to change Flags field ?

// Set the compressed flag. That is zlib style.
  Section.Flags |= ELF::SHF_COMPRESSED;

So do I correctly understand that you want me to create setter for this patch and remove it and getter in following commit ? what is the point for that ? May be let me just make Flags public at first ?

1024 ↗

(On Diff #57614)

will do. quick search shows that both way are used, but I would agree here.

[• rafael]

lib/MC/MCSectionELF.cpp

162 ↗

(On Diff #57614)

Lets please do this first with just a getter and setter to make the patch easy to read.

[grimar]

• Addressed review comments.

lib/MC/ELFObjectWriter.cpp

576 ↗

(On Diff #57614)

Returned back original code, added setter for Flags instead.

lib/MC/MCSectionELF.cpp

162 ↗

(On Diff #57614)

Done.

[• rafael]

LGTM. Thanks!

[davide]

lgtm

[grimar]

Had to temporaliry revert this, but also found another issue with landing, details are here: D20466
Working on that.

llvm/trunk/include/llvm/MC/MCContext.h

MCSectionELF *getELFSection(StringRef Section, unsigned Type,
                                 const char *BeginSymName,
                                 const MCSectionELF *Associated);
 
     MCSectionELF *createELFRelSection(StringRef Name, unsigned Type,
                                       unsigned Flags, unsigned EntrySize,
                                       const MCSymbolELF *Group,
                                       const MCSectionELF *Associated);
 
-    void renameELFSection(MCSectionELF *Section, StringRef Name);
-
     MCSectionELF *createELFGroupSection(const MCSymbolELF *Group);
 
     MCSectionCOFF *getCOFFSection(StringRef Section, unsigned Characteristics,
                                   SectionKind Kind, StringRef COMDATSymName,
                                   int Selection,
                                   unsigned UniqueID = GenericSectionID,
                                   const char *BeginSymName = nullptr);
 

llvm/trunk/include/llvm/MC/MCSectionELF.h

MCSectionELF(StringRef Section, unsigned type, unsigned flags, SectionKind K,
                MCSymbol *Begin, const MCSectionELF *Associated)
       : MCSection(SV_ELF, K, Begin), SectionName(Section), Type(type),
         Flags(flags), UniqueID(UniqueID), EntrySize(entrySize), Group(group),
         Associated(Associated) {
     if (Group)
       Group->setIsSignature();
   }
 
-  void setSectionName(StringRef Name) { SectionName = Name; }
-
 public:
   ~MCSectionELF();
 
   /// Decides whether a '.section' directive should be printed before the
   /// section name
   bool ShouldOmitSectionDirective(StringRef Name, const MCAsmInfo &MAI) const;
 
   StringRef getSectionName() const { return SectionName; }
   unsigned getType() const { return Type; }
   unsigned getFlags() const { return Flags; }
   unsigned getEntrySize() const { return EntrySize; }
+  void setFlags(unsigned F) { Flags = F; }
   const MCSymbolELF *getGroup() const { return Group; }
 
   void PrintSwitchToSection(const MCAsmInfo &MAI, raw_ostream &OS,
                             const MCExpr *Subsection) const override;
   bool UseCodeAlign() const override;
   bool isVirtualSection() const override;
 
   bool isUnique() const { return UniqueID != ~0U; }

llvm/trunk/lib/MC/ELFObjectWriter.cpp

ELFObjectWriter::createRelocationSection(MCContext &Ctx,
 
   MCSectionELF *RelaSection = Ctx.createELFRelSection(
       RelaSectionName, hasRelocationAddend() ? ELF::SHT_RELA : ELF::SHT_REL,
       Flags, EntrySize, Sec.getGroup(), &Sec);
   RelaSection->setAlignment(is64Bit() ? 8 : 4);
   return RelaSection;
 }
 
-// Include the debug info compression header:
-// "ZLIB" followed by 8 bytes representing the uncompressed size of the section,
-// useful for consumers to preallocate a buffer to decompress into.
-static bool
-prependCompressionHeader(uint64_t Size,
-                         SmallVectorImpl<char> &CompressedContents) {
-  const StringRef Magic = "ZLIB";
-  if (Size <= Magic.size() + sizeof(Size) + CompressedContents.size())
-    return false;
-  if (sys::IsLittleEndianHost)
-    sys::swapByteOrder(Size);
-  CompressedContents.insert(CompressedContents.begin(),
-                            Magic.size() + sizeof(Size), 0);
-  std::copy(Magic.begin(), Magic.end(), CompressedContents.begin());
-  std::copy(reinterpret_cast<char *>(&Size),
-            reinterpret_cast<char *>(&Size + 1),
-            CompressedContents.begin() + Magic.size());
-  return true;
-}
-
 void ELFObjectWriter::writeSectionData(const MCAssembler &Asm, MCSection &Sec,
                                        const MCAsmLayout &Layout) {
   MCSectionELF &Section = static_cast<MCSectionELF &>(Sec);
   StringRef SectionName = Section.getSectionName();
 
   // Compressing debug_frame requires handling alignment fragments which is
   // more work (possibly generalizing MCAssembler.cpp:writeFragment to allow
   // for writing to arbitrary buffers) for little benefit.
void ELFObjectWriter::writeSectionData(const MCAssembler &Asm, MCSection &Sec,
   zlib::Status Success = zlib::compress(
       StringRef(UncompressedData.data(), UncompressedData.size()),
       CompressedContents);
   if (Success != zlib::StatusOK) {
     getStream() << UncompressedData;
     return;
   }
 
-  if (!prependCompressionHeader(UncompressedData.size(), CompressedContents)) {
+  uint64_t HdrSize =
+      is64Bit() ? sizeof(ELF::Elf32_Chdr) : sizeof(ELF::Elf64_Chdr);
+  if (UncompressedData.size() <= HdrSize + CompressedContents.size()) {
     getStream() << UncompressedData;
     return;
   }
-  Asm.getContext().renameELFSection(&Section,
-                                    (".z" + SectionName.drop_front(1)).str());
+
+  // Set the compressed flag. That is zlib style.
+  Section.setFlags(Section.getFlags() | ELF::SHF_COMPRESSED);
+
+  // Platform specific header is followed by compressed data.
+  if (is64Bit()) {
+    // Write Elf64_Chdr header.
+    write(static_cast<ELF::Elf64_Word>(ELF::ELFCOMPRESS_ZLIB));
+    write(static_cast<ELF::Elf64_Word>(0)); // ch_reserved field.
+    write(static_cast<ELF::Elf64_Xword>(UncompressedData.size()));
+    write(static_cast<ELF::Elf64_Xword>(Sec.getAlignment()));
+  } else {
+    // Write Elf32_Chdr header otherwise.
+    write(static_cast<ELF::Elf32_Word>(ELF::ELFCOMPRESS_ZLIB));
+    write(static_cast<ELF::Elf32_Word>(UncompressedData.size()));
+    write(static_cast<ELF::Elf32_Word>(Sec.getAlignment()));
+  }
+
   getStream() << CompressedContents;
 }
 
 void ELFObjectWriter::WriteSecHdrEntry(uint32_t Name, uint32_t Type,
                                        uint64_t Flags, uint64_t Address,
                                        uint64_t Offset, uint64_t Size,
                                        uint32_t Link, uint32_t Info,
                                        uint64_t Alignment,

llvm/trunk/lib/MC/MCContext.cpp

MCSectionMachO *MCContext::getMachOSection(StringRef Segment, StringRef Section,
   if (BeginSymName)
     Begin = createTempSymbol(BeginSymName, false);
 
   // Otherwise, return a new section.
   return Entry = new (MachOAllocator.Allocate()) MCSectionMachO(
              Segment, Section, TypeAndAttributes, Reserved2, Kind, Begin);
 }
 
-void MCContext::renameELFSection(MCSectionELF *Section, StringRef Name) {
-  StringRef GroupName;
-  if (const MCSymbol *Group = Section->getGroup())
-    GroupName = Group->getName();
-
-  unsigned UniqueID = Section->getUniqueID();
-  ELFUniquingMap.erase(
-      ELFSectionKey{Section->getSectionName(), GroupName, UniqueID});
-  auto I = ELFUniquingMap.insert(std::make_pair(
-                                     ELFSectionKey{Name, GroupName, UniqueID},
-                                     Section))
-               .first;
-  StringRef CachedName = I->first.SectionName;
-  const_cast<MCSectionELF *>(Section)->setSectionName(CachedName);
-}
-
 MCSectionELF *MCContext::createELFRelSection(StringRef Name, unsigned Type,
                                              unsigned Flags, unsigned EntrySize,
                                              const MCSymbolELF *Group,
                                              const MCSectionELF *Associated) {
   StringMap<bool>::iterator I;
   bool Inserted;
   std::tie(I, Inserted) = ELFRelSecNames.insert(std::make_pair(Name, true));
 

llvm/trunk/test/MC/ELF/compression.s

 // RUN: llvm-mc -filetype=obj -compress-debug-sections -triple x86_64-pc-linux-gnu < %s -o %t
 // RUN: llvm-objdump -s %t | FileCheck %s
 // RUN: llvm-dwarfdump -debug-dump=info %t | FileCheck --check-prefix=INFO %s
 // RUN: llvm-mc -filetype=obj -compress-debug-sections -triple i386-pc-linux-gnu < %s \
 // RUN:     | llvm-readobj -symbols - | FileCheck --check-prefix=386-SYMBOLS %s
+// RUN: llvm-readobj -sections %t | FileCheck --check-prefix=ZLIB %s
 
 // REQUIRES: zlib
 
-// CHECK: Contents of section .zdebug_line:
-// Check for the 'ZLIB' file magic at the start of the section only
-// CHECK-NEXT: ZLIB
-// CHECK-NOT: ZLIB
+// Check that debug_line section was not renamed, so it is
+// zlib-style, not zlib-gnu one. Check that SHF_COMPRESSED was set.
+// ZLIB:      Section {
+// ZLIB:        Index:
+// ZLIB:        Name: .debug_line
+// ZLIB-NEXT:   Type: SHT_PROGBITS
+// ZLIB-NEXT:   Flags [
+// ZLIB-NEXT:     SHF_COMPRESSED
+// ZLIB-NEXT:   ]
 
 // Don't compress small sections, such as this simple debug_abbrev example
 // CHECK: Contents of section .debug_abbrev:
 // CHECK-NOT: ZLIB
 // CHECK-NOT: Contents of
 
 // CHECK: Contents of section .debug_info:
 
 // FIXME: Handle compressing alignment fragments to support compressing debug_frame
 // CHECK: Contents of section .debug_frame:
 // CHECK-NOT: ZLIB
 // CHECK: Contents of
 
 // Decompress one valid dwarf section just to check that this roundtrips
 // INFO: 0x00000000: Compile Unit: length = 0x0000000c version = 0x0004 abbr_offset = 0x0000 addr_size = 0x08 (next unit at 0x00000010)
 
 // In x86 32 bit named symbols are used for temporary symbols in merge
 // sections, so make sure we handle symbols inside compressed sections
 // 386-SYMBOLS: Name: .Linfo_string0
 // 386-SYMBOLS-NOT: }
-// 386-SYMBOLS: Section: .zdebug_str
+// 386-SYMBOLS: Section: .debug_str
 
   .section  .debug_line,"",@progbits
 
   .section  .debug_abbrev,"",@progbits
 .Lsection_abbrev:
   .byte 1                       # Abbreviation Code
   .byte 17                      # DW_TAG_compile_unit
   .byte 0                       # DW_CHILDREN_no