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

[Namhyung Kim via Phabricator via llvm-commits]

namhyung added inline comments.


================
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;
----------------
MaskRay wrote:
> namhyung wrote:
> > I've found a bug in this code - it should check equality.
> The convention does not place parens around `!=`
Yeah, but I thought it'd good to be symmetric to the RHS.


================
Comment at: llvm/include/llvm/Object/ELF.h:185
   std::vector<Elf_Shdr> FakeSections;
+  std::vector<char> FakeSectionStrings;
 
----------------
jhenderson wrote:
> MaskRay wrote:
> > 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).
> I disagree that it adds complexity. It seems more intuitive to do `StrTab.add(someString)` than `std::copy(someString.begin(), someString.end(), std::back_inserter(StrTab));` followed by a null terminator addition. I do agree that the optimizations are unnecessary. Maybe there's room for a "simple" string builder class that does none of the deduplication/optimization?
I agree with @jhenderson.  The API in the StringTableBuilder is simpler and more intuitive to me.  Also `finalizeInOrder()` is even not trying to optimize it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131290