ELF: more section creation cleanup

[labath]

Summary

This patch attempts to move as much code as possible out of the
CreateSections function to make room for future improvements there. Some
of this may be slightly over-engineered (VMAddressProvider), but I
wanted to keep the logic of this function very simple, because once I
start taking segment headers into acount (as discussed in D55356), the
function is going to grow significantly.

While in there, I also added tests for various bits of functionality.

This should be NFC, except that I changed the order of hac^H^Heuristicks
for determining section type slightly. Previously, name-based deduction
(.symtab -> symtab) would take precedence over type-based (SHT_SYMTAB ->
symtab) one. In fact we would assert if we ran into a .text section with
type SHT_SYMTAB. Though unlikely to matter in practice, this order
seemed wrong to me, so I have inverted it.

[krytarowski]

I recommend joerg@ as a reviewer for ELF specifics.

[clayborg]

Looks good.

[labath]

Kamil, Joerg: most of this is fairly lldb-specific and boring. I just added you for the .symtab thingy, just in case you know of anyone using SHT_SYMTAB sections which are not called .symtab (or conversely, using the name .symtab for non-symbol-table-related things).

lit/Modules/ELF/compressed-sections.yaml

- Name:            .hello_elf
     Content:         010000000800000001000000789c5330700848286898000009c802c1
   - Name:            .bogus
     Type:            SHT_PROGBITS
     Flags:           [ SHF_COMPRESSED ]
     Content:         deadbeefbaadf00d
 
 # CHECK: Name: .hello_elf
 # CHECK-NEXT: Type: regular
-# CHECK-NEXT: Thread specific: no
-# CHECK-NEXT: VM size: 0
+# CHECK: VM size: 0
 # CHECK-NEXT: File size: 28
 # CHECK-NEXT: Data:
 # CHECK-NEXT: 20304050 60708090
 
 # CHECK: Name: .bogus
 # CHECK-NEXT: Type: regular
-# CHECK-NEXT: Thread specific: no
-# CHECK-NEXT: VM size: 0
+# CHECK: VM size: 0
 # CHECK-NEXT: File size: 8
 # CHECK-NEXT: Data: ()

lit/Modules/ELF/section-addresses.yaml

+# RUN: yaml2obj %s > %t
+# RUN: lldb-test object-file %t | FileCheck %s
+
+# CHECK-LABEL: Name: .one
+# CHECK: VM address: 0x0
+
+# CHECK-LABEL: Name: .nonalloc
+# CHECK: VM address: 0x0
+
+# CHECK-LABEL: Name: .two
+# CHECK: VM address: 0x8
+
+# CHECK-LABEL: Name: .three
+# CHECK: VM address: 0xc
+
+# CHECK-LABEL: Name: .four
+# CHECK: VM address: 0xc
+
+# CHECK-LABEL: Name: .five
+# CHECK: VM address: 0x1000
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+  Entry:           0x00000000000007A0
+Sections:
+  - Name:            .one
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    AddressAlign:    0x0000000000000004
+    Content:         DEADBEEFBAADF00D
+  - Name:            .nonalloc
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x0000000000000004
+    Content:         DEADBEEFBAADF00D
+  - Name:            .two
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    AddressAlign:    0x0000000000000004
+    Content:         DE
+  - Name:            .three
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    AddressAlign:    0x0000000000000004
+  - Name:            .four
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    AddressAlign:    0x0000000000000004
+    Content:         DEADBEEFBAADF00D
+  - Name:            .five
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    AddressAlign:    0x0000000000001000
+    Content:         DEADBEEFBAADF00D
+...

lit/Modules/ELF/section-permissions.yaml

+# RUN: yaml2obj %s > %t
+# RUN: lldb-test object-file %t | FileCheck %s
+
+# CHECK-LABEL: Name: .r-x
+# CHECK: Permissions: r-x
+#
+# CHECK-LABEL: Name: .rw-
+# CHECK: Permissions: rw-
+
+# CHECK-LABEL: Name: .---
+# CHECK: Permissions: ---
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_DYN
+  Machine:         EM_X86_64
+  Entry:           0x00000000000007A0
+Sections:
+  - Name:            .r-x
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Content:         DEADBEEFBAADF00D
+  - Name:            .rw-
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    AddressAlign:    0x0000000000000004
+    Content:         DEADBEEFBAADF00D
+  - Name:            .---
+    Type:            SHT_PROGBITS
+    AddressAlign:    0x0000000000000001
+    Content:         DEADBEEFBAADF00D
+...

lit/Modules/ELF/section-types-edgecases.yaml

+# This test doesn't attempt to mandate a specific section classification. It is
+# here to document the existing behavior and to make sure we don't do something
+# completely crazy (like crashing).
+
+# RUN: yaml2obj %s > %t
+# RUN: lldb-test object-file %t | FileCheck %s
+
+# The section is called .data, but it has the SHF_EXECINSTR flag set. Have
+# the flag take precedence over the name.
+# CHECK-LABEL: Name: .data
+# CHECK-NEXT: Type: code
+
+# Section type (SHT_SYMTAB) takes precedence over name-based deduction.
+# CHECK-LABEL: Name: .text
+# CHECK-NEXT: Type: elf-symbol-table
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_DYN
+  Machine:         EM_X86_64
+  Entry:           0x00000000000007A0
+Sections:
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_EXECINSTR, SHF_ALLOC ]
+    AddressAlign:    0x0000000000000004
+    Content:         DEADBEEFBAADF00D
+  - Name:            .text
+    Type:            SHT_SYMTAB
+    Flags:           [ ]
+    AddressAlign:    0x0000000000000004
+    Content:         DEADBEEFBAADF00D
+...

lit/Modules/ELF/section-types.yaml

 # CHECK-NEXT: Type: code
 
 # CHECK-LABEL: Name: .data
 # CHECK-NEXT: Type: data
 # CHECK: Thread specific: no
 
 # CHECK-LABEL: Name: .tdata
 # CHECK-NEXT: Type: data
-# CHECK-NEXT: Thread specific: yes
+# CHECK: Thread specific: yes
 
 # CHECK-LABEL: Name: .tbss
 # CHECK-NEXT: Type: zero-fill
-# CHECK-NEXT: Thread specific: yes
+# CHECK: Thread specific: yes
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS64
   Data:            ELFDATA2LSB
   Type:            ET_DYN
   Machine:         EM_X86_64
   Entry:           0x00000000000007A0

lit/Modules/MachO/subsections.yaml

 # RUN: yaml2obj %s > %t
 # RUN: lldb-test object-file %t | FileCheck %s
 
 #CHECK:     Showing 2 sections
 #CHECK-NEXT:  Index: 0
 #CHECK-NEXT:  Name: __PAGEZERO
 #CHECK-NEXT:  Type: container
+#CHECK-NEXT:  Permissions: ---
 #CHECK-NEXT:  Thread specific: no
+#CHECK-NEXT:  VM address: 0x0
 #CHECK-NEXT:  VM size: 4294967296
 #CHECK-NEXT:  File size: 0
 #CHECK-NEXT:  There are no subsections
 #
 #CHECK:       Index: 1
 #CHECK-NEXT:  Name: __TEXT
 #CHECK-NEXT:  Type: container
+#CHECK-NEXT:  Permissions: r-x
 #CHECK-NEXT:  Thread specific: no
+#CHECK-NEXT:  VM address: 0x100000000
 #CHECK-NEXT:  VM size: 4096
 #CHECK-NEXT:  File size: 4096
 #CHECK-NEXT:  Showing 3 subsections
 #CHECK-NEXT:    Index: 0
 #CHECK-NEXT:    Name: __text
 #CHECK-NEXT:    Type: code
+#CHECK-NEXT:    Permissions: r-x
 #CHECK-NEXT:    Thread specific: no
+#CHECK-NEXT:    VM address: 0x100000f30
 #CHECK-NEXT:    VM size: 22
 #CHECK-NEXT:    File size: 22
 #
 #CHECK:         Index: 1
 #CHECK-NEXT:    Name: __unwind_info
 #CHECK-NEXT:    Type: compact-unwind
+#CHECK-NEXT:    Permissions: r-x
 #CHECK-NEXT:    Thread specific: no
+#CHECK-NEXT:    VM address: 0x100000f48
 #CHECK-NEXT:    VM size: 76
 #CHECK-NEXT:    File size: 76
 #
 #CHECK:         Index: 2
 #CHECK-NEXT:    Name: __eh_frame
 #CHECK-NEXT:    Type: eh-frame
+#CHECK-NEXT:    Permissions: r-x
 #CHECK-NEXT:    Thread specific: no
+#CHECK-NEXT:    VM address: 0x100000f98
 #CHECK-NEXT:    VM size: 104
 #CHECK-NEXT:    File size: 104
 
 --- !mach-o
 FileHeader:      
   magic:           0xFEEDFACF
   cputype:         0x01000007
   cpusubtype:      0x00000003

source/Plugins/ObjectFile/ELF/ObjectFileELF.h

private:
   /// Returns true iff the headers have been successfully parsed.
   bool ParseProgramHeaders();
 
   /// Parses all section headers present in this object file and populates
   /// m_section_headers.  This method will compute the header list only once.
   /// Returns the number of headers parsed.
   size_t ParseSectionHeaders();
 
+  lldb::SectionType GetSectionType(const ELFSectionHeaderInfo &H) const;
+
   static void ParseARMAttributes(lldb_private::DataExtractor &data,
                                  uint64_t length,
                                  lldb_private::ArchSpec &arch_spec);
 
   /// Parses the elf section headers and returns the uuid, debug link name,
   /// crc, archspec.
   static size_t GetSectionHeaderInfo(SectionHeaderColl &section_headers,
                                      lldb_private::DataExtractor &object_data,

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

lldb::user_id_t ObjectFileELF::GetSectionIndexByName(const char *name) {
   if (!name || !name[0] || !ParseSectionHeaders())
     return 0;
   for (size_t i = 1; i < m_section_headers.size(); ++i)
     if (m_section_headers[i].section_name == ConstString(name))
       return i;
   return 0;
 }
 
-static SectionType getSectionType(llvm::StringRef Name) {
+static SectionType GetSectionTypeFromName(llvm::StringRef Name) {
   return llvm::StringSwitch<SectionType>(Name)
       .Case(".ARM.exidx", eSectionTypeARMexidx)
       .Case(".ARM.extab", eSectionTypeARMextab)
       .Cases(".bss", ".tbss", eSectionTypeZeroFill)
       .Cases(".data", ".tdata", eSectionTypeData)
       .Case(".debug_abbrev", eSectionTypeDWARFDebugAbbrev)
       .Case(".debug_abbrev.dwo", eSectionTypeDWARFDebugAbbrevDwo)
       .Case(".debug_addr", eSectionTypeDWARFDebugAddr)
return llvm::StringSwitch<SectionType>(Name)
       .Case(".debug_types", eSectionTypeDWARFDebugTypes)
       .Case(".eh_frame", eSectionTypeEHFrame)
       .Case(".gnu_debugaltlink", eSectionTypeDWARFGNUDebugAltLink)
       .Case(".gosymtab", eSectionTypeGoSymtab)
       .Case(".text", eSectionTypeCode)
       .Default(eSectionTypeOther);
 }
 
+SectionType ObjectFileELF::GetSectionType(const ELFSectionHeaderInfo &H) const {
+  switch (H.sh_type) {
+  case SHT_PROGBITS:
+    if (H.sh_flags & SHF_EXECINSTR)
+      return eSectionTypeCode;
+    break;
+  case SHT_SYMTAB:
+    return eSectionTypeELFSymbolTable;
+  case SHT_DYNSYM:
+    return eSectionTypeELFDynamicSymbols;
+  case SHT_RELA:
+  case SHT_REL:
+    return eSectionTypeELFRelocationEntries;
+  case SHT_DYNAMIC:
+    return eSectionTypeELFDynamicLinkInfo;
+  }
+  SectionType Type = GetSectionTypeFromName(H.section_name.GetStringRef());
+  if (Type == eSectionTypeOther) {
+    // the kalimba toolchain assumes that ELF section names are free-form.
+    // It does support linkscripts which (can) give rise to various
+    // arbitrarily named sections being "Code" or "Data".
+    Type = kalimbaSectionType(m_header, H);
+  }
+  return Type;
+}
+
+static uint32_t GetTargetByteSize(SectionType Type, const ArchSpec &arch) {
+  switch (Type) {
+  case eSectionTypeData:
+  case eSectionTypeZeroFill:
+    return arch.GetDataByteSize();
+  case eSectionTypeCode:
+    return arch.GetCodeByteSize();
+  default:
+    return 1;
+  }
+}
+
+static Permissions GetPermissions(const ELFSectionHeader &H) {
+  Permissions Perm = Permissions(0);
+  if (H.sh_flags & SHF_ALLOC)
+    Perm |= ePermissionsReadable;
+  if (H.sh_flags & SHF_WRITE)
+    Perm |= ePermissionsWritable;
+  if (H.sh_flags & SHF_EXECINSTR)
+    Perm |= ePermissionsExecutable;
+  return Perm;
+}
+
+namespace {
+// (Unlinked) ELF object files usually have 0 for every section address, meaning
+// we need to compute synthetic addresses in order for "file addresses" from
+// different sections to not overlap. This class handles that logic.
+class VMAddressProvider {
+  bool m_synthesizing;
+  addr_t m_next;
+
+public:
+  VMAddressProvider(ObjectFile::Type Type)
+      : m_synthesizing(Type == ObjectFile::Type::eTypeObjectFile), m_next(0) {}
+
+  std::pair<addr_t, addr_t> GetAddressAndSize(const ELFSectionHeader &H) {
+    addr_t address = H.sh_addr;
+    addr_t size = H.sh_flags & SHF_ALLOC ? H.sh_size : 0;
+    if (m_synthesizing && (H.sh_flags & SHF_ALLOC)) {
+      m_next = llvm::alignTo(m_next, std::max<addr_t>(H.sh_addralign, 1));
+      address = m_next;
+      m_next += size;
+    }
+    return {address, size};
+  }
+};
+}
+
 void ObjectFileELF::CreateSections(SectionList &unified_section_list) {
   if (!m_sections_ap.get() && ParseSectionHeaders()) {
     m_sections_ap.reset(new SectionList());
 
-    // Object files frequently have 0 for every section address, meaning we
-    // need to compute synthetic addresses in order for "file addresses" from
-    // different sections to not overlap
-    bool synthaddrs = (CalculateType() == ObjectFile::Type::eTypeObjectFile);
-    uint64_t nextaddr = 0;
-
+    VMAddressProvider address_provider(CalculateType());
     for (SectionHeaderCollIter I = m_section_headers.begin();
          I != m_section_headers.end(); ++I) {
       const ELFSectionHeaderInfo &header = *I;
 
       ConstString &name = I->section_name;
       const uint64_t file_size =
           header.sh_type == SHT_NOBITS ? 0 : header.sh_size;
-      const uint64_t vm_size = header.sh_flags & SHF_ALLOC ? header.sh_size : 0;
 
-      SectionType sect_type = getSectionType(name.GetStringRef());
+      addr_t vm_addr, vm_size;
+      std::tie(vm_addr, vm_size) = address_provider.GetAddressAndSize(header);
 
-      bool is_thread_specific = header.sh_flags & SHF_TLS;
-      const uint32_t permissions =
-          ((header.sh_flags & SHF_ALLOC) ? ePermissionsReadable : 0u) |
-          ((header.sh_flags & SHF_WRITE) ? ePermissionsWritable : 0u) |
-          ((header.sh_flags & SHF_EXECINSTR) ? ePermissionsExecutable : 0u);
-      switch (header.sh_type) {
-      case SHT_SYMTAB:
-        assert(sect_type == eSectionTypeOther);
-        sect_type = eSectionTypeELFSymbolTable;
-        break;
-      case SHT_DYNSYM:
-        assert(sect_type == eSectionTypeOther);
-        sect_type = eSectionTypeELFDynamicSymbols;
-        break;
-      case SHT_RELA:
-      case SHT_REL:
-        assert(sect_type == eSectionTypeOther);
-        sect_type = eSectionTypeELFRelocationEntries;
-        break;
-      case SHT_DYNAMIC:
-        assert(sect_type == eSectionTypeOther);
-        sect_type = eSectionTypeELFDynamicLinkInfo;
-        break;
-      }
-
-      if (eSectionTypeOther == sect_type) {
-        // the kalimba toolchain assumes that ELF section names are free-form.
-        // It does support linkscripts which (can) give rise to various
-        // arbitrarily named sections being "Code" or "Data".
-        sect_type = kalimbaSectionType(m_header, header);
-      }
-
-      // In common case ELF code section can have arbitrary name (for example,
-      // we can specify it using section attribute for particular function) so
-      // assume that section is a code section if it has SHF_EXECINSTR flag set
-      // and has SHT_PROGBITS type.
-      if (eSectionTypeOther == sect_type &&
-          llvm::ELF::SHT_PROGBITS == header.sh_type &&
-          (header.sh_flags & SHF_EXECINSTR)) {
-        sect_type = eSectionTypeCode;
-      }
+      SectionType sect_type = GetSectionType(header);
 
       const uint32_t target_bytes_size =
-          (eSectionTypeData == sect_type || eSectionTypeZeroFill == sect_type)
-              ? m_arch_spec.GetDataByteSize()
-              : eSectionTypeCode == sect_type ? m_arch_spec.GetCodeByteSize()
-                                              : 1;
+          GetTargetByteSize(sect_type, m_arch_spec);
+
       elf::elf_xword log2align =
           (header.sh_addralign == 0) ? 0 : llvm::Log2_64(header.sh_addralign);
 
-      uint64_t addr = header.sh_addr;
-
-      if ((header.sh_flags & SHF_ALLOC) && synthaddrs) {
-          nextaddr =
-              (nextaddr + header.sh_addralign - 1) & ~(header.sh_addralign - 1);
-          addr = nextaddr;
-          nextaddr += vm_size;
-      }
-
       SectionSP section_sp(new Section(
           GetModule(), // Module to which this section belongs.
           this, // ObjectFile to which this section belongs and should read
                 // section data from.
           SectionIndex(I),     // Section ID.
           name,                // Section name.
           sect_type,           // Section type.
-          addr,                // VM address.
+          vm_addr,             // VM address.
           vm_size,             // VM size in bytes of this section.
           header.sh_offset,    // Offset of this section in the file.
           file_size,           // Size of the section as found in the file.
           log2align,           // Alignment of the section
           header.sh_flags,     // Flags for this section.
           target_bytes_size)); // Number of host bytes per target byte
 
-      section_sp->SetPermissions(permissions);
-      if (is_thread_specific)
-        section_sp->SetIsThreadSpecific(is_thread_specific);
+      section_sp->SetPermissions(GetPermissions(header));
+      section_sp->SetIsThreadSpecific(header.sh_flags & SHF_TLS);
       m_sections_ap->AddSection(section_sp);
     }
   }
 
   // For eTypeDebugInfo files, the Symbol Vendor will take care of updating the
   // unified section list.
   if (GetType() != eTypeDebugInfo)
     unified_section_list = *m_sections_ap;

tools/lldb-test/lldb-test.cpp

 #include "lldb/Symbol/LineTable.h"
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/TypeList.h"
 #include "lldb/Symbol/VariableList.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/CleanUp.h"
 #include "lldb/Utility/DataExtractor.h"
+#include "lldb/Utility/State.h"
 #include "lldb/Utility/StreamString.h"
 
 #include "llvm/ADT/IntervalMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/Path.h"
Printer.formatLine("Showing {0} {1}sections", Count,
                      is_subsection ? "sub" : "");
   for (size_t I = 0; I < Count; ++I) {
     auto S = List.GetSectionAtIndex(I);
     assert(S);
     AutoIndent Indent(Printer, 2);
     Printer.formatLine("Index: {0}", I);
     Printer.formatLine("Name: {0}", S->GetName().GetStringRef());
     Printer.formatLine("Type: {0}", S->GetTypeAsCString());
+    Printer.formatLine("Permissions: {0}", GetPermissionsAsCString(S->GetPermissions()));
     Printer.formatLine("Thread specific: {0:y}", S->IsThreadSpecific());
+    Printer.formatLine("VM address: {0:x}", S->GetFileAddress());
     Printer.formatLine("VM size: {0}", S->GetByteSize());
     Printer.formatLine("File size: {0}", S->GetFileSize());
 
     if (opts::object::SectionContents) {
       DataExtractor Data;
       S->GetSectionData(Data);
       ArrayRef<uint8_t> Bytes = {Data.GetDataStart(), Data.GetDataEnd()};
       Printer.formatBinary("Data: ", Bytes, 0);