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

[Namhyung Kim via Phabricator via llvm-commits]

namhyung added inline comments.


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


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section.test:8
+# RUN: llvm-objdump -d %t | FileCheck %s
+
+# CHECK:       <>:
----------------
MaskRay wrote:
> Consider adding the two lines
> ```
> # CHECK:       Disassembly of section :
> # CHECK-EMPTY:
> ```
Ok, will add.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section.test:18
+
+# RUN: llvm-objdump -d --start-address=0xffffffff00000000 \
+# RUN:   --stop-address=0xffffffff00000004 %t | \
----------------
MaskRay wrote:
> There is a warning (may be spurious) but you don't need to fix it in this patch.
> 
> Use `2>&1 | ` and test the warning to catch changes when someone fixes it.
Sure.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section.test:22
+
+# RANGE:       55                    pushq   %rbp
+# RANGE-NEXT:  48 89 e5              movq    %rsp, %rbp
----------------
MaskRay wrote:
> Better to add needed anchor lines (like `<>:` you added for the test above) before `55 pushq %rbp`.
> 
> It may not look great, but the intention is to catch display change which others may improve.
Will add.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1134
 
-static void disassembleObject(const Target *TheTarget, const ObjectFile *Obj,
+static void createFakeELFSections(ObjectFile *Obj) {
+  assert(Obj->isELF());
----------------
MaskRay wrote:
> For non-null pointers, the convention in llvm-project is to use references. The llvm-objdump code traditionally did not follow the best practice. I just pushed a commit to migrate a bunch of functions. You will need to rebase and change these to references.
I see, I will make the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128705