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

[Namhyung Kim via Phabricator via llvm-commits]

namhyung added a comment.

> This needs a test. I suggest llvm/test/tools/llvm-objdump/X86/disassemble-no-section.test.

Yes, I'd like to add a test but it requires a specific binary file (ET_CORE extracted by perf tools).  I'm seeing that most of the test uses a text input.  Could you please provide an example how to use a binary file in the test?



================
Comment at: llvm/include/llvm/Object/ELF.h:187-189
+  void createFakeSections();
+  std::unique_ptr<Elf_Shdr[]> FakeSec;
+  int NumFakeSec;
----------------
jhenderson wrote:
> 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.
> 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).

llvm-objdump calls Obj->sections() multiple times even only for disassembly.  It's to check section addresses, to print outputs and other cases.

> Unrelated, but I feel like it'd be cleaner to use a std::vector rather than an array and a separate number. 

Yeah, I'd also love to do that.  I was unsure if std::vector provides a contiguous memory for ArrayRef but it seems C++ guarantees it.  Will change!


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


================
Comment at: llvm/include/llvm/Object/ELF.h:772
 
+/// This function creates fake section headers from program headers.
+/// This is for linux perf tools because it copies a part of kcore
----------------
MaskRay wrote:
> In binutils, BFD synthesizes more sections, but for disassembly purpose just the executable ones are sufficient. I think the code is fine.
> 
> The comment I am thinking is:
> 
> Used by llvm-objdump -d (which needs sections for disassembly) to disassemble objects without a section header table (e.g. ET_CORE objects analyzed by linux perf).
Thanks, I will update the comment as you wrote.


================
Comment at: llvm/include/llvm/Object/ELF.h:782
+  NumFakeSec = 0;
+  for (auto Phdr : *PhdrsOrErr) {
+    if (!(Phdr.p_type & ELF::PT_LOAD))
----------------
jhenderson wrote:
> 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).
Looks good.


================
Comment at: llvm/include/llvm/Object/ELF.h:787
+      continue;
+    NumFakeSec++;
+  }
----------------
MaskRay wrote:
> pre-increment
Will change.


================
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;
----------------
jhenderson wrote:
> You could factor this check into a simple function to avoid repeating it.
Sure, will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128705