[llvm-size][libobject] Add explicit "inTextSegment" methods similar to "isText" section methods to calculate size correctly.

[rupprecht]

Summary

llvm-size uses "isText()" etc. which seem to indicate whether the section contains code-like things, not whether or not it will actually go in the text segment when in a fully linked executable.

The unit test added (elf-sizes.test) shows some types of sections that cause discrepencies versus the GNU size tool. llvm-size is not correctly reporting sizes of things mapping to text/data segments, at least for ELF files.

This fixes pr38723.

[echristo]

I think this is the right direction, couple comment:

• More detail in the comments for the *Segment methods. Maybe something elaborating about link/execution time or something else? Examples as well :)
• While it's fairly obvious it'll work, a couple of sections with the right flags but not named the standard thing would be good for the test. .text.unlikely and .data1 maybe?

[MaskRay]

include/llvm/Object/ELFObjectFile.h

767

The parentheses outside of ! are redundant.

GNU size uses Berkeley format by default (static int berkeley_format = BSD_DEFAULT;)
I'd say it uses a weird counting https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=binutils/size.c;h=3c72e484752d0272a24ac628f497f89ecf36d547;hb=refs/heads/master#l459

459 if ((flags & SEC_CODE) != 0 || (flags & SEC_READONLY) != 0)
460 textsize += size;
461 else if ((flags & SEC_HAS_CONTENTS) != 0)
462 datasize += size;
463 else
464 bsssize += size;

Basically SHF_EXECINSTR sections (.text) + !SHF_WRITE sections (.rodata .eh_frame ...).

I feel the name inTextSegment is a bit misleading. How about isBerkeleyText?

inDataSegment may be renamed to isBerkeleyData.

[MaskRay]

include/llvm/Object/ELFObjectFile.h

767

The Berkeley format is not that weird. It uses some simple condition to approximate how the traditional linkers partition sections into segments.

With the advent of fine-grained segments (split of R and RX): -z separate-code in ld.bfd and lld's default (unless --no-rosegment), the computation may be less relevant. isBerkeleyText makes more sense to me.

[rupprecht]

• Remove useless parens
• Rename inXXXSegment->isBerkeleyXXX
• Clarify segment comments
• Add test cases that ensure section flags are used instead of section names

[rupprecht]

Sorry, for not getting to this patch sooner, I dropped it due to vacation + some unexpected things... I'm still eager to get this patch in :)

In D54369#1293994, @echristo wrote:

I think this is the right direction, couple comment:

• More detail in the comments for the *Segment methods. Maybe something elaborating about link/execution time or something else? Examples as well :)

Expanded a bit, I can add more if this isn't enough

• While it's fairly obvious it'll work, a couple of sections with the right flags but not named the standard thing would be good for the test. .text.unlikely and .data1 maybe?

Done, added a couple examples both ways (named .text.* but not actually text, and named .something_random but is actually data)

(Addressed Ray's comments too)

[MaskRay]

include/llvm/Object/ObjectFile.h

111

Typo here. initialized -> uninitialized

https://en.wikipedia.org/wiki/.bss "BSS reserves a block of uninitialized data"

include/llvm/Object/ELFObjectFile.h

protected:
   std::error_code getSectionContents(DataRefImpl Sec,
                                      StringRef &Res) const override;
   uint64_t getSectionAlignment(DataRefImpl Sec) const override;
   bool isSectionCompressed(DataRefImpl Sec) const override;
   bool isSectionText(DataRefImpl Sec) const override;
   bool isSectionData(DataRefImpl Sec) const override;
   bool isSectionBSS(DataRefImpl Sec) const override;
   bool isSectionVirtual(DataRefImpl Sec) const override;
+  bool inTextSegment(DataRefImpl Sec) const override;
+  bool inDataSegment(DataRefImpl Sec) const override;
   relocation_iterator section_rel_begin(DataRefImpl Sec) const override;
   relocation_iterator section_rel_end(DataRefImpl Sec) const override;
   std::vector<SectionRef> dynamic_relocation_sections() const override;
   section_iterator getRelocatedSection(DataRefImpl Sec) const override;
 
   void moveRelocationNext(DataRefImpl &Rel) const override;
   uint64_t getRelocationOffset(DataRefImpl Rel) const override;
   symbol_iterator getRelocationSymbol(DataRefImpl Rel) const override;

 }
 
 template <class ELFT>
 bool ELFObjectFile<ELFT>::isSectionVirtual(DataRefImpl Sec) const {
   return getSection(Sec)->sh_type == ELF::SHT_NOBITS;
 }
 
 template <class ELFT>
+bool ELFObjectFile<ELFT>::inTextSegment(DataRefImpl Sec) const {
+  return getSection(Sec)->sh_flags & ELF::SHF_ALLOC &&
+         (getSection(Sec)->sh_flags & ELF::SHF_EXECINSTR ||
+          (!(getSection(Sec)->sh_flags & ELF::SHF_WRITE)));
+}
+
+template <class ELFT>
+bool ELFObjectFile<ELFT>::inDataSegment(DataRefImpl Sec) const {
+  const Elf_Shdr *EShdr = getSection(Sec);
+  return !inTextSegment(Sec) && EShdr->sh_type != ELF::SHT_NOBITS &&
+         EShdr->sh_flags & ELF::SHF_ALLOC;
+}
+
+template <class ELFT>
 relocation_iterator
 ELFObjectFile<ELFT>::section_rel_begin(DataRefImpl Sec) const {
   DataRefImpl RelData;
   auto SectionsOrErr = EF.sections();
   if (!SectionsOrErr)
     return relocation_iterator(RelocationRef());
   uintptr_t SHT = reinterpret_cast<uintptr_t>((*SectionsOrErr).begin());
   RelData.d.a = (Sec.p - SHT) / EF.getHeader()->e_shentsize;

include/llvm/Object/ObjectFile.h

public:
   uint64_t getIndex() const;
   uint64_t getSize() const;
   std::error_code getContents(StringRef &Result) const;
 
   /// Get the alignment of this section as the actual value (not log 2).
   uint64_t getAlignment() const;
 
   bool isCompressed() const;
+  /// Whether this section contains instructions.
   bool isText() const;
+  /// Whether this section contains data, not instructions.
   bool isData() const;
+  /// Whether this section contains BSS initialized data.
   bool isBSS() const;
   bool isVirtual() const;
   bool isBitcode() const;
   bool isStripped() const;
 
+  /// Whether this section will be placed in the text segment.
+  bool inTextSegment() const;
+  /// Whether this section will be placed in the data segment (and is not BSS).
+  bool inDataSegment() const;
+  /// Whether this is a BSS section that will be placed in the data segment.
+  bool inBSSSegment() const;
+
   bool containsSymbol(SymbolRef S) const;
 
   relocation_iterator relocation_begin() const;
   relocation_iterator relocation_end() const;
   iterator_range<relocation_iterator> relocations() const {
     return make_range(relocation_begin(), relocation_end());
   }
   section_iterator getRelocatedSection() const;
protected:
   virtual bool isSectionCompressed(DataRefImpl Sec) const = 0;
   virtual bool isSectionText(DataRefImpl Sec) const = 0;
   virtual bool isSectionData(DataRefImpl Sec) const = 0;
   virtual bool isSectionBSS(DataRefImpl Sec) const = 0;
   // A section is 'virtual' if its contents aren't present in the object image.
   virtual bool isSectionVirtual(DataRefImpl Sec) const = 0;
   virtual bool isSectionBitcode(DataRefImpl Sec) const;
   virtual bool isSectionStripped(DataRefImpl Sec) const;
+  virtual bool inTextSegment(DataRefImpl Sec) const;
+  virtual bool inDataSegment(DataRefImpl Sec) const;
+  virtual bool inBSSSegment(DataRefImpl Sec) const;
   virtual relocation_iterator section_rel_begin(DataRefImpl Sec) const = 0;
   virtual relocation_iterator section_rel_end(DataRefImpl Sec) const = 0;
   virtual section_iterator getRelocatedSection(DataRefImpl Sec) const;
 
   // Same as above for RelocationRef.
   friend class RelocationRef;
   virtual void moveRelocationNext(DataRefImpl &Rel) const = 0;
   virtual uint64_t getRelocationOffset(DataRefImpl Rel) const = 0;

 inline bool SectionRef::isBitcode() const {
   return OwningObject->isSectionBitcode(SectionPimpl);
 }
 
 inline bool SectionRef::isStripped() const {
   return OwningObject->isSectionStripped(SectionPimpl);
 }
 
+inline bool SectionRef::inTextSegment() const {
+  return OwningObject->inTextSegment(SectionPimpl);
+}
+
+inline bool SectionRef::inDataSegment() const {
+  return OwningObject->inDataSegment(SectionPimpl);
+}
+
+inline bool SectionRef::inBSSSegment() const {
+  return OwningObject->inBSSSegment(SectionPimpl);
+}
+
 inline relocation_iterator SectionRef::relocation_begin() const {
   return OwningObject->section_rel_begin(SectionPimpl);
 }
 
 inline relocation_iterator SectionRef::relocation_end() const {
   return OwningObject->section_rel_end(SectionPimpl);
 }
 

lib/Object/ObjectFile.cpp

bool ObjectFile::isSectionBitcode(DataRefImpl Sec) const {
   StringRef SectName;
   if (!getSectionName(Sec, SectName))
     return SectName == ".llvmbc";
   return false;
 }
 
 bool ObjectFile::isSectionStripped(DataRefImpl Sec) const { return false; }
 
+bool ObjectFile::inTextSegment(DataRefImpl Sec) const {
+  return isSectionText(Sec);
+}
+
+bool ObjectFile::inDataSegment(DataRefImpl Sec) const {
+  return isSectionData(Sec);
+}
+
+bool ObjectFile::inBSSSegment(DataRefImpl Sec) const {
+  return isSectionBSS(Sec);
+}
+
 section_iterator ObjectFile::getRelocatedSection(DataRefImpl Sec) const {
   return section_iterator(SectionRef(Sec, this));
 }
 
 Triple ObjectFile::makeTriple() const {
   Triple TheTriple;
   auto Arch = getArch();
   TheTriple.setArch(Triple::ArchType(Arch));

test/tools/llvm-size/X86/elf-sizes.test

+# RUN: yaml2obj %s > %t.o
+# RUN: llvm-size -B %t.o | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .bss
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_ALLOC, SHF_WRITE ]
+    Size:            1
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Size:            2
+  - Name:            .eh_frame
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    Size:            4
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_WRITE ]
+    Size:            8
+  - Name:            .debug_info
+    Type:            SHT_PROGBITS
+    Flags:           [ ]
+    Size:            16
+  - Name:            .init_array
+    Type:            SHT_INIT_ARRAY
+    Flags:           [ SHF_ALLOC, SHF_WRITE ]
+    Size:            32
+
+# text is .text, .eh_frame: 2 + 4 = 6
+# data is .data, .init_array: 8 + 32 = 40
+# bss is .bss: 1
+# total: 47
+# unaccounted for (not affecting total) is .debug_info
+
+# CHECK: text data bss dec
+# CHECK: 6    40   1   47

test/tools/llvm-size/X86/ignore-sections.s

 // SYSV-NEXT:    .data                  4      0
 // SYSV-NEXT:    .bss                   4      0
 // SYSV-NEXT:    .comment               5      0
 // SYSV-NEXT:    foo                    4      0
 // SYSV-NEXT:    .eh_frame             48      0
 // SYSV-NEXT:    Total                 69
 
 // BSD:        text    data     bss     dec     hex filename
-// BSD-NEXT:      4       4       4      12       c {{[ -\(\)_A-Za-z0-9.\\/:]+}}
-// BSD-NEXT:      4       4       4      12       c (TOTALS)
+// BSD-NEXT:     52       4       4      60      3c {{[ -\(\)_A-Za-z0-9.\\/:]+}}
+// BSD-NEXT:     52       4       4      60      3c (TOTALS)

tools/llvm-size/llvm-size.cpp

else if (OutputFormat == sysv) {
     // displays the cumulative size for each section type.
     uint64_t total_text = 0;
     uint64_t total_data = 0;
     uint64_t total_bss = 0;
 
     // Make one pass over the section table to calculate sizes.
     for (const SectionRef &Section : Obj->sections()) {
       uint64_t size = Section.getSize();
-      bool isText = Section.isText();
-      bool isData = Section.isData();
-      bool isBSS = Section.isBSS();
+      bool isText = Section.inTextSegment();
+      bool isData = Section.inDataSegment();
+      bool isBSS = Section.inBSSSegment();
       if (isText)
         total_text += size;
       else if (isData)
         total_data += size;
       else if (isBSS)
         total_bss += size;
     }