[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

Why do you need to change this?

lib/MC/MCSectionELF.cpp

164

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

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

So nice to see this go :-)

lib/MC/ELFObjectWriter.cpp

1027

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

You can delete this one now.

[grimar]

lib/MC/MCSectionELF.cpp

164

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

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

Done.

lib/MC/ELFObjectWriter.cpp

576

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

1027

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

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

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

lib/MC/MCSectionELF.cpp

162

Unrelated.

[grimar]

lib/MC/ELFObjectWriter.cpp

576

Please see my comment below.

lib/MC/MCSectionELF.cpp

162

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

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

1020

static_cast<> instead of C-style cast?

[davide]

lib/MC/ELFObjectWriter.cpp

576

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

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 ?

1020

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

[• rafael]

lib/MC/MCSectionELF.cpp

162

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

Returned back original code, added setter for Flags instead.

lib/MC/MCSectionELF.cpp

162

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.

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);
 

include/llvm/MC/MCSectionELF.h

 class MCSectionELF final : public MCSection {
   /// This is the name of the section.  The referenced memory is owned by
   /// TargetLoweringObjectFileELF's ELFUniqueMap.
   StringRef SectionName;
 
   /// This is the sh_type field of a section, drawn from the enums below.
   unsigned Type;
 
-  /// This is the sh_flags field of a section, drawn from the enums below.
-  unsigned Flags;
-
   unsigned UniqueID;
 
   /// The size of each entry in this section. This size only makes sense for
   /// sections that contain fixed-sized entries. If a section does not contain
   /// fixed-sized entries 'EntrySize' will be 0.
   unsigned EntrySize;
 
   const MCSymbolELF *Group;
 
   /// Depending on the type of the section this is sh_link or sh_info.
   const MCSectionELF *Associated;
 
 private:
   friend class MCContext;
   MCSectionELF(StringRef Section, unsigned type, unsigned flags, SectionKind K,
                unsigned entrySize, const MCSymbolELF *group, unsigned UniqueID,
                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; }
   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; }
   unsigned getUniqueID() const { return UniqueID; }
 
   const MCSectionELF *getAssociatedSection() const { return Associated; }
 
   static bool classof(const MCSection *S) {
     return S->getVariant() == SV_ELF;
   }
+
+  /// This is the sh_flags field of a section, drawn from the enums below.
+  unsigned Flags;
 };
 
 } // end namespace llvm
 
 #endif

lib/MC/ELFObjectWriter.cpp

bool ELFObjectWriter::shouldRelocateWithSymbol(const MCAssembler &Asm,
   // If a relocation points to a mergeable section, we have to be careful.
   // If the offset is zero, a relocation with the section will encode the
   // same information. With a non-zero offset, the situation is different.
   // For example, a relocation can point 42 bytes past the end of a string.
   // If we change such a relocation to use the section, the linker would think
   // that it pointed to another string and subtracting 42 at runtime will
   // produce the wrong value.
   auto &Sec = cast<MCSectionELF>(Sym->getSection());
-  unsigned Flags = Sec.getFlags();
-  if (Flags & ELF::SHF_MERGE) {
+  if (Sec.Flags & ELF::SHF_MERGE) {
     if (C != 0)
       return true;
 
     // It looks like gold has a bug (http://sourceware.org/PR16794) and can
     // only handle section relocations to mergeable sections if using RELA.
     if (!hasRelocationAddend())
       return true;
   }
 
   // Most TLS relocations use a got, so they need the symbol. Even those that
   // are just an offset (@tpoff), require a symbol in gold versions before
   // 5efeedf61e4fe720fd3e9a08e6c91c10abb66d42 (2014-09-26) which fixed
   // http://sourceware.org/PR16773.
-  if (Flags & ELF::SHF_TLS)
+  if (Sec.Flags & ELF::SHF_TLS)
     return true;
 
   // If the symbol is a thumb function the final relocation must set the lowest
   // bit. With a symbol that is done by just having the symbol have that bit
   // set, so we would lose the bit if we relocated with the section.
   // FIXME: We could use the section but add the bit to the relocation value.
   if (Asm.isThumbFunc(Sym))
     return true;
ELFObjectWriter::createRelocationSection(MCContext &Ctx,
 
   unsigned EntrySize;
   if (hasRelocationAddend())
     EntrySize = is64Bit() ? sizeof(ELF::Elf64_Rela) : sizeof(ELF::Elf32_Rela);
   else
     EntrySize = is64Bit() ? sizeof(ELF::Elf64_Rel) : sizeof(ELF::Elf32_Rel);
 
   unsigned Flags = 0;
-  if (Sec.getFlags() & ELF::SHF_GROUP)
+  if (Sec.Flags & ELF::SHF_GROUP)
     Flags = ELF::SHF_GROUP;
 
   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.Flags |= ELF::SHF_COMPRESSED;
+
+  // Platform specific header is followed by compressed data.
+  uint32_t Type = ELF::ELFCOMPRESS_ZLIB;
+  if (sys::IsLittleEndianHost)
+    sys::swapByteOrder(Type);
+
+  if (is64Bit()) {
+    getStream() << (uint32_t)0; // ch_reserved field.
+
+    uint64_t Size = UncompressedData.size();
+    if (sys::IsLittleEndianHost)
+      sys::swapByteOrder(Size);
+    getStream() << Size;
+
+    uint64_t Alignment = Sec.getAlignment();
+    if (sys::IsLittleEndianHost)
+      sys::swapByteOrder(Alignment);
+    getStream() << Alignment;
+  } else {
+    uint32_t Size = UncompressedData.size();
+    if (sys::IsLittleEndianHost)
+      sys::swapByteOrder(Size);
+    getStream() << Size;
+
+    uint32_t Alignment = Sec.getAlignment();
+    if (sys::IsLittleEndianHost)
+      sys::swapByteOrder(Alignment);
+    getStream() << Alignment;
+  }
+
   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,
case ELF::SHT_GROUP:
     break;
   }
 
   if (TargetObjectWriter->getEMachine() == ELF::EM_ARM &&
       Section.getType() == ELF::SHT_ARM_EXIDX)
     sh_link = SectionIndexMap.lookup(Section.getAssociatedSection());
 
   WriteSecHdrEntry(StrTabBuilder.getOffset(Section.getSectionName()),
-                   Section.getType(), Section.getFlags(), 0, Offset, Size,
-                   sh_link, sh_info, Section.getAlignment(),
-                   Section.getEntrySize());
+                   Section.getType(), Section.Flags, 0, Offset, Size, sh_link,
+                   sh_info, Section.getAlignment(), Section.getEntrySize());
 }
 
 void ELFObjectWriter::writeSectionHeader(
     const MCAsmLayout &Layout, const SectionIndexMapTy &SectionIndexMap,
     const SectionOffsetsTy &SectionOffsets) {
   const unsigned NumSections = SectionTable.size();
 
   // Null section first.

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));
 

lib/MC/MCELFStreamer.cpp

 void MCELFStreamer::EmitLabel(MCSymbol *S) {
   auto *Symbol = cast<MCSymbolELF>(S);
   assert(Symbol->isUndefined() && "Cannot define a symbol twice!");
 
   MCObjectStreamer::EmitLabel(Symbol);
 
   const MCSectionELF &Section =
       static_cast<const MCSectionELF &>(*getCurrentSectionOnly());
-  if (Section.getFlags() & ELF::SHF_TLS)
+  if (Section.Flags & ELF::SHF_TLS)
     Symbol->setType(ELF::STT_TLS);
 }
 
 void MCELFStreamer::EmitAssemblerFlag(MCAssemblerFlag Flag) {
   // Let the target do whatever target specific stuff it needs to do.
   getAssembler().getBackend().handleAssemblerFlag(Flag);
   // Do any generic stuff we need to do.
   switch (Flag) {

lib/MC/MCSectionELF.cpp

void MCSectionELF::PrintSwitchToSection(const MCAsmInfo &MAI,
 
   if (Subsection) {
     OS << "\t.subsection\t";
     Subsection->print(OS, &MAI);
     OS << '\n';
   }
 }
 
-bool MCSectionELF::UseCodeAlign() const {
-  return getFlags() & ELF::SHF_EXECINSTR;
-}
+bool MCSectionELF::UseCodeAlign() const { return Flags & ELF::SHF_EXECINSTR; }
 
-bool MCSectionELF::isVirtualSection() const {
-  return getType() == ELF::SHT_NOBITS;
-}
+bool MCSectionELF::isVirtualSection() const { return Flags == ELF::SHT_NOBITS; }

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