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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In D82479#2114637 <https://reviews.llvm.org/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?



================
Comment at: llvm/tools/llvm-size/llvm-size.cpp:199
+  if (Section.getIndex() == getShstrndx(Obj))
+    return false;
+
----------------
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).


================
Comment at: llvm/tools/llvm-size/llvm-size.cpp:215
+    return false;
   case ELF::SHT_STRTAB:
   case ELF::SHT_REL:
----------------
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`?




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82479/new/

https://reviews.llvm.org/D82479