[PATCH] D128705: [llvm-objdump] Create fake sections for a ELF core file

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELF.h:187-189
+  void createFakeSections();
+  std::unique_ptr<Elf_Shdr[]> FakeSec;
+  int NumFakeSec;
----------------
As I understand it, the only reason you have these members is so that you only need to create them once, even if there are multiple calls to `sections`. Is that right? If so, are there actually cases where there are multiple such calls that make sense to have fake sections? (I know little about ET_CORE files or how BFD creates fake sections, so this may be fine).

Unrelated, but I feel like it'd be cleaner to use a std::vector rather than an array and a separate number. This would also allow you to create the fake sections in a single loop rather than needing to calculate the array size and then populate it (just use vector.push_back each time you encounter a relevant Phdr, and voila, you have an array of the right size with the right elements with only one loop). Furthermore, I believe this would allow you to avoid the explicit copy constructor, since the fake section vector would be inherently copyable, unlike the unique_ptr array.


================
Comment at: llvm/include/llvm/Object/ELF.h:188
+  void createFakeSections();
+  std::unique_ptr<Elf_Shdr[]> FakeSec;
+  int NumFakeSec;
----------------
I think this would be better spelt "FakeSections" without the abbreviation, primarily since it is plural.


================
Comment at: llvm/include/llvm/Object/ELF.h:782
+  NumFakeSec = 0;
+  for (auto Phdr : *PhdrsOrErr) {
+    if (!(Phdr.p_type & ELF::PT_LOAD))
----------------
This loop could become `std::count(PhdrsOrErr->begin(), PhdrsOrErr->end(), [](const Elf_Phdr &) { return (Phdr.p_type & ELF::PT_LOAD) && (Phdr.p_flags & ELF::PF_X); });`

Personally, I'd find it neater (but it may be superfluous - see above re. std::vector usage).


================
Comment at: llvm/include/llvm/Object/ELF.h:796-799
+    if (!(Phdr.p_type & ELF::PT_LOAD))
+      continue;
+    if (!(Phdr.p_flags & ELF::PF_X))
+      continue;
----------------
You could factor this check into a simple function to avoid repeating it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128705