[PATCH] D128705: [llvm-objdump] Create fake sections for a ELF core file
Namhyung Kim via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 1 15:10:59 PDT 2022
namhyung added a comment.
Thanks for your detailed review! Really appreciated!
================
Comment at: llvm/include/llvm/Object/ELF.h:768
+template <class ELFT> void ELFFile<ELFT>::createFakeSections() {
+ if (getHeader().e_type != ELF::ET_CORE || !FakeSections.empty())
+ return;
----------------
MaskRay wrote:
> This should not be restricted to ET_CORE. `llvm-strip --strip-sections` processed files can be disassembled by objdump -d
Hmm.. ok.
================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:462
+ void createFakeSections() {
+ if (getEType() == ELF::ET_CORE) {
+ EF.createFakeSections();
----------------
jhenderson wrote:
> I believe this check is now redundant?
Right, will remove.
================
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:
> 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?
I guess normal core files don't have the contents (code) since it's in the binary. But linux perf tools generate a core file with binary to disassemble a kernel function easily. That's why I need to add content to phdr (now it works with section fill though).
================
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
----------------
namhyung wrote:
> jhenderson wrote:
> > 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?
> I guess normal core files don't have the contents (code) since it's in the binary. But linux perf tools generate a core file with binary to disassemble a kernel function easily. That's why I need to add content to phdr (now it works with section fill though).
Ok, will change.
================
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
----------------
jhenderson wrote:
> Is there any particular reason to do two separate llvm-objdump invocations?
>
> Do you actually need the explicit --start-address and --stop-address options?
> Is there any particular reason to do two separate llvm-objdump invocations?
@maskray asked to test it with `-h` option too.
> Do you actually need the explicit --start-address and --stop-address options?
No, but that's how the liinux perf tools run it. I'd like to make it as same as the actual use case.
================
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
+
----------------
jhenderson wrote:
> Does the GNU equivalent have no section name here?
No, they generate section name based on p_type and index. Probably I can add it as a follow up.
================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:12
+
+# DISAS: <>:
+# DISAS-NEXT: 55 pushq %rbp
----------------
jhenderson wrote:
> 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?
> We usually add additional spacing to ensure lines without -NEXT suffixes line up with those that do (see inline edit).
Thanks, will change.
> Again, does this style match GNU?
Do you mean symbol name? They just show the same name of the section. Not sure it's from a (fake) symbol or falls back to section name.
================
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
----------------
jhenderson wrote:
> To reduce test noise, this could be just `PF_X`.
It's the same as kcore extracted by linux perf tools and I'd like have the same environment as much as possible.
================
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
----------------
jhenderson wrote:
> Is the high VAddr actually important to the test?
Not really, but again, I just wanted to have a similar binary.
================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:37
+ VAddr: 0xFFFFFFFF8103398C
+ PAddr: 0x0
+ Offset: 0x1000
----------------
jhenderson wrote:
> Is this relevant? If not, I believe it can be omitted.
Then it'd have the same value as VAddr. FYI, kcore files have 0 for PAddr. But for tests, it doesn't matter.
================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:38-41
+ Offset: 0x1000
+ FileSize: 10
+ MemSize: 10
+ Align: 0x1000
----------------
jhenderson wrote:
> 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.
For this test, they don't matter. It follows values in a kcore.
================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:45
+...
+
----------------
jhenderson wrote:
> Nit: too many blank lines at EOF (the file should end with exactly one \n).
Will remove.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1920
+ if (Obj->isELF() && Obj->sections().empty())
+ createFakeELFSections(const_cast<ObjectFile *>(Obj));
+
----------------
jhenderson wrote:
> 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?
Not really, `dumpObject` already has a non-const ObjectFile pointer. I'll make the change.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128705/new/
https://reviews.llvm.org/D128705
More information about the llvm-commits
mailing list