[PATCH] D131290: [llvm-objdump] Create name for fake sections

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/Object/ELF.h:656
+  if (!Index) {
+    // no section string table.
+    if (!FakeSectionStrings.empty())
----------------
`// There is no section name string table. Return FakeSectionStrings which is non-empty if we have created fake sections.`


================
Comment at: llvm/include/llvm/Object/ELF.h:657
+    // no section string table.
+    if (!FakeSectionStrings.empty())
+      return FakeSectionStrings;
----------------
Just return `FakeSectionStrings;` and delete `return ""`


================
Comment at: llvm/include/llvm/Object/ELF.h:784
   for (auto Phdr : *PhdrsOrErr) {
-    if (!(Phdr.p_type & ELF::PT_LOAD) || !(Phdr.p_flags & ELF::PF_X))
+    if ((Phdr.p_type != ELF::PT_LOAD) || !(Phdr.p_flags & ELF::PF_X)) {
+      ++Idx;
----------------
namhyung wrote:
> I've found a bug in this code - it should check equality.
The convention does not place parens around `!=`


================
Comment at: llvm/include/llvm/Object/ELF.h:185
   std::vector<Elf_Shdr> FakeSections;
+  std::vector<char> FakeSectionStrings;
 
----------------
namhyung wrote:
> MaskRay wrote:
> > Use `SmallString<0> FakeSectionStrings`.
> > 
> > Prefer `SmallString` because `std::string` has a small string optimization which isn't useful for this case.
> But I think it'd be better to use StringTableBuilder as @jhenderson said.
I think `StringTableBuilder` just adds complexity/overhead in this case. `StringTableBuilder` cannot deduplicate any string here (and the code does not need to optimize  for size).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131290