Page MenuHomePhabricator

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

Authored by gbreynoo on Jun 24 2020, 9:48 AM.

Details

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

Diff Detail

Event Timeline

gbreynoo created this revision.Jun 24 2020, 9:48 AM
Herald added a project: Restricted Project. · View Herald Transcript

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.

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?

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
191

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?

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

grimar added inline comments.Jun 26 2020, 12:58 AM
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 updated this revision to Diff 273761.Jun 26 2020, 10:00 AM

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.

gbreynoo marked 3 inline comments as done.Jun 26 2020, 10:01 AM
gbreynoo updated this revision to Diff 273769.Jun 26 2020, 10:08 AM
MaskRay added a comment.EditedJun 26 2020, 10:08 AM

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 accepted this revision.EditedJun 30 2020, 1:52 AM

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

This revision is now accepted and ready to land.Jun 30 2020, 1:52 AM
This revision was automatically updated to reflect the committed changes.

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.

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.