[llvm-size] Output REL, RELA and STRTAB sections in some cases

[gbreynoo]

Summary

gnu size has a number of special cases regarding REL, RELA and STRTAB sections being considered in size output. To avoid unnecessary complexity this commit makes llvm size outputs these sections in cases they have the SHF_ALLOC flag. See related bugzilla:

https://bugs.llvm.org/show_bug.cgi?id=42934

[MaskRay]

I'm not sure always outputting these sections is correct, below is a diff for a potential change that does not copy the complex rules:

I am questioning why omitting these sections in considered incorrect. SHT_STRTAB is not usually SHF_ALLOC. If we can find cases where GNU size -A emits SHT_REL and SHT_RELA, we can delete the three lines:

case ELF::SHT_STRTAB:
case ELF::SHT_REL:
case ELF::SHT_RELA:

even if we can't delete SHT_SYMTAB and SHT_NULL.

[gbreynoo]

There may be some confusion on my part. My understanding is that size is used to measure the memory footprint whilst there are other utilities for viewing information regarding the files themselves. See https://bugs.llvm.org/show_bug.cgi?id=46299 for a bug involving the use of size and the expectation of its output.

Is this also your expectation of output?

.dynstr occupies memory during execution but I don't think string tables should be omitted otherwise. This is why I followed Grimar’s suggestion regarding SHF_ALLOC. In order to simplify the code we could only output sections with SHF_ALLOC flag and avoid the type dependant behaviour all together?

[grimar]

In D82479#2114637, @gbreynoo wrote:

Is this also your expectation of output?
...
This is why I followed Grimar’s suggestion regarding SHF_ALLOC. In order to simplify the code we could only output sections with SHF_ALLOC flag and avoid the type dependant behaviour all together?

And that is why I've suggested it. I agree with your expectations of the output. For me it sounds reasonable given the behavior of GNU size you've described in the bug.
@MaskRay?

llvm/tools/llvm-size/llvm-size.cpp

199

I'd make the isAllocatable just to return what it says. I.e:

static bool isAllocatableSec(ObjectFile *Obj, SectionRef Section) {
  assert(Obj->isELF());
  ELFSectionRef ElfSection = static_cast<ELFSectionRef>(Section);
  return ElfSection.getFlags() & ELF::SHF_ALLOC;
}

And you can have another method, like isShStrTab to check if a section is one with the index == e_shstrndx (but see below).

215

Probably you shouldn't mix SHT_SYMTAB vs SHT_REL[A]. It doesn't seem correct to
consider a non-alloc SHT_REL[A] even when it has an index that is equal to e_shstrndx?
(In this case an object is just malformed).

Then it could be something like:

case ELF::SHT_STRTAB:
  return isAllocatable(Obj, Section) || isShStrTab(Obj, Section);
case ELF::SHT_REL:
case ELF::SHT_RELA: 
  return isAllocatable(Obj, Section);

BTW, why`.shstrtab` is considered? Can we just omit the logic related to e_shstrndx?

[grimar]

llvm/tools/llvm-size/llvm-size.cpp

199

Or, given that considerForSize() already only used for ELFs,
you can probably just inline it, like:

static bool considerForSize(ObjectFile *Obj, SectionRef Section) {
  if (!Obj->isELF())
    return true;

  ELFSectionRef ElfSection = static_cast<ELFSectionRef>(Section);
  switch (ElfSection .getType()) {
    ...
    case ELF::SHT_STRTAB:
      return ElfSection.getFlags() & ELF::SHF_ALLOC;
  }
}

[gbreynoo]

Thanks for the comments Grimar. I have removed the check specifically for .shstrtab as it should not have the SHF_ALLOC flag anyway and simplified the area as suggested.

[MaskRay]

I've verified that GNU size -A:

• omits .symtab/.strtab/.shstrtab . This probably only applies to non-SHF_ALLOC sections
• omits non-SHF_ALLOC .rel{,a}.*
• does not omit non-SHF_ALLOC .comment, __pachable_function_entries, etc

From the first two items, I can understand that GNU size has the intention to omit non-SHF_ALLOC sections. However, the third item appears to conflict with the rule. I kind of believe that omitting .symtab/.strtab/.shstrtab may not be an intentional decision because they are special in BFD. The usual section iteration loop may not apply to them (https://sourceware.org/bugzilla/show_bug.cgi?id=26168 ) The logic appears to be some inconsistent rules which I don't very like much.

If you indeed have a specific need that llvm-size -A should match GNU size -A output, I think this patch is fine and is moving toward that goal. If there is no specific need, I'd like to remove SHT_REL & SHT_RELA from the conditions. If we don't want to hide .symtab/.strtab/.shstrtab , that will be even better for me. I can't think of why showing additional 3 entries can be a problem.

[grimar]

This patch makes the output closer to GNU size, so LGTM.
I think intentional further deviation from the GNU output can be done separately (if we want it).

[MaskRay]

In D82479#2122112, @grimar wrote:

This patch makes the output closer to GNU size, so LGTM.
I think intentional further deviation from the GNU output can be done separately (if we want it).

I think I don't feel too happy that the commit introduced complexity before we have a better understanding of GNU size's exact behaviors and before we reach a consensus that we do want to emulate GNU size in this (IMHO) very minor edge cases.

The motivation is not very clearly expressed.

gnu size should be GNU size.

[gbreynoo]

Apologies Maskray, I misunderstood your previous comment and should not have committed before you also accepted the change. If you would prefer this change to be reverted whilst discussion continues I would be happy to do so.

I think that this change better matches what users expect from llvm-size, and considering the small size and impact of this change that it is worth including. I agree it is useful to investigate the current GNU size behaviour and work out what the intended behaviour is, from what I've seen however it looks to be overly complex and inconsistent. A wider discussion may be required for what llvm-size should output as to fit with user expectation, as seen in the bug I linked above there is some confusion.

In response to your previous comment suggesting to always output SHT_REL & SHT_RELA and potentially string tables, I don't think these should be output unless allocatable. These sections would be added to the size total in cases in which they do not occupy memory.

llvm/test/tools/llvm-size/output-alloc.test

+## gnu size has a number of special cases regarding REL, RELA and STRTAB sections being considered in size output. 
+## To avoid unnecessary complexity llvm size outputs these sections in cases they have the SHF_ALLOC flag.
+
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-size --format=sysv %t1 | FileCheck %s
+
+# CHECK:      section     size   addr
+# CHECK-NEXT: .rela.alloc
+# CHECK-NEXT: .rel.alloc
+# CHECK-NEXT: .strtab.alloc
+# CHECK-NEXT: Total
+# CHECK-EMPTY:
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name:  .rela.alloc
+    Type:  SHT_RELA
+    Flags: [ SHF_ALLOC ]
+  - Name:  .rel.alloc
+    Type:  SHT_REL
+    Flags: [ SHF_ALLOC ]
+  - Name:  .strtab.alloc
+    Type:  SHT_STRTAB
+    Flags: [ SHF_ALLOC ]
+  - Name:  .rela
+    Type:  SHT_RELA
+    Flags: []
+  - Name:  .rel
+    Type:  SHT_REL
+    Flags: []
+  - Name:  .strtab
+    Type:  SHT_STRTAB
+    Flags: []

llvm/tools/llvm-size/llvm-size.cpp

   switch (static_cast<ELFSectionRef>(Section).getType()) {
   case ELF::SHT_NULL:
   case ELF::SHT_SYMTAB:
+    return false;
   case ELF::SHT_STRTAB:
   case ELF::SHT_REL:
   case ELF::SHT_RELA:
-    return false;
+    ELFSectionRef ElfSection = static_cast<ELFSectionRef>(Section);
+    return ElfSection.getFlags() & ELF::SHF_ALLOC;
   }
   return true;
 }