[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/ELFObjectFile.h:462
+  void createFakeSections() {
+    if (getEType() == ELF::ET_CORE) {
+      EF.createFakeSections();
----------------
I believe this check is now redundant?


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:264
   bool ContentValid = false;
+  bool FakeSectionsUsed = false;
 
----------------
namhyung wrote:
> namhyung wrote:
> > jhenderson wrote:
> > > I think this boolean is probably superfluous: you coudl check within `createFakeSections` whether the FakeSections vector is empty and just end early if it is.
> > But the fake section vector is a private member of ELFFile so we cannot check it here.  Do you want me to add a public function in ELFFile to check it?
> I'll just move the check to ELFFile.
Yes, that's what I meant.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:1
+# This test checks -d disassembles a kcore file extracted by linux perf tools
+# which doesn't have section headers
----------------
jhenderson wrote:
> Newer tests use `##` for comments to help distinguish them from lit and FileCheck directives.
> 
> Comments should end with a full stop.
What's special about kcore files as opposed to any other ET_CORE files?


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:1-2
+# This test checks -d disassembles a kcore file extracted by linux perf tools
+# which doesn't have section headers
+# RUN: yaml2obj %s -o %t
----------------
Newer tests use `##` for comments to help distinguish them from lit and FileCheck directives.

Comments should end with a full stop.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:4-6
+# RUN: llvm-objdump -h %t | FileCheck %s --check-prefix HEADER
+# RUN: llvm-objdump -d --start-address=0xffffffff8103398c \
+# RUN: --stop-address=0xffffffff81033996 %t | FileCheck %s --check-prefix DISAS
----------------
Is there any particular reason to do two separate llvm-objdump invocations?

Do you actually need the explicit --start-address and --stop-address options?


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:10
+# HEADER-NEXT: Idx Name          Size     VMA              Type
+# HEADER-NEXT:   0               0000000a ffffffff8103398c TEXT
+
----------------
Does the GNU equivalent have no section name here?


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:12
+
+# DISAS:  <>:
+# DISAS-NEXT:  55                    pushq   %rbp
----------------
We usually add additional spacing to ensure lines without -NEXT suffixes line up with those that do (see inline edit).

Again, does this style match GNU?


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:35
+  - Type:            PT_LOAD
+    Flags:           [ PF_X, PF_W, PF_R ]
+    VAddr:           0xFFFFFFFF8103398C
----------------
To reduce test noise, this could be just `PF_X`.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:36
+    Flags:           [ PF_X, PF_W, PF_R ]
+    VAddr:           0xFFFFFFFF8103398C
+    PAddr:           0x0
----------------
Is the high VAddr actually important to the test?


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:37
+    VAddr:           0xFFFFFFFF8103398C
+    PAddr:           0x0
+    Offset:          0x1000
----------------
Is this relevant? If not, I believe it can be omitted.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:38-41
+    Offset:          0x1000
+    FileSize:        10
+    MemSize:         10
+    Align:           0x1000
----------------
I believe the Align field is unnecessary for the test. I believe the Offset and FileSize are derived by the associated "section" too, so those should be omittable too. It's potentially the same for MemSize, but I'm not certain.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:45
+...
+
----------------
Nit: too many blank lines at EOF (the file should end with exactly one \n).


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1920
+  if (Obj->isELF() && Obj->sections().empty())
+    createFakeELFSections(const_cast<ObjectFile *>(Obj));
+
----------------
Is it possible to just remove the const from the input parameter, to avoid the const_cast, or does that require further changes in the call tree?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128705