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

Mon Jul 4 00:00:17 PDT 2022

jhenderson added inline comments.

================
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
----------------
namhyung wrote:
> 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.
Okay, thanks for the explanation. As @MaskRay points out, llvm-strip can result in binaries without section headers, which could also benefit from being disassembled. Consequently, this isn't a feature specific to kcore files. I'd suggest renaming the test to simply `disassemble-no-section-headers.test`. You should also update the comment to say something like: "This test checks -d disassembles an ELF file without section headers. Such files include kcore files extracted by linux perf tools, or executables with section headers stripped by e.g. llvm-strip --strip-sections." (The second sentence is entirely optional, but it's nice to have the original motivating example).

================
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
----------------
namhyung wrote:
> 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.
> > Is there any particular reason to do two separate llvm-objdump invocations?
> 
> @maskray asked to test it with `-h` option too.

Yeah, I understand that. I meant you could do something like `llvm-objdump -d -h ...` rather than two separate executions of llvm-objdump. This would help the test performance. On the other hand, there's an argument for splitting out the -h case into an entirely separate file, because it's not about disassembly.

> > 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.

Okay, fair enough. In that case, I think it would be good to have test cases both with and without those options. This is to show that llvm-objdump will dump the all the relevant bytes, but also that it can be restricted by address for kcore files. What do you think?

================
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
+
----------------
namhyung wrote:
> 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.
Sounds good.

================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:12
+
+# DISAS:  <>:
+# DISAS-NEXT:  55                    pushq   %rbp
----------------
namhyung wrote:
> 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.
I guess my main objection is to the `<>:` format, which just looks weird. I guess we should either completely omit it or synthesize a name for it. It might be that the section name addition above will cover this though automatically.

================
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
----------------
namhyung wrote:
> 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.
The risk is that by having extra stuff in the file, you obscure what's actually important for the test case. We try to keep things as minimal as possible in general in our testing. PF_R and PF_W have no bearing on how llvm-objdump operates, so they are unnecessary and just obscure the important bit (PF_X), meaning people may not realise that the PF_X is actually the important bit.

As a spin-off thought, should we have some content covered by a phdr without PF_X, to show that this isn't disassembled? Also, what should happen with `-D` (which disassembles non-executable stuff too)?

================
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
----------------
namhyung wrote:
> jhenderson wrote:
> > Is the high VAddr actually important to the test?
> Not really, but again, I just wanted to have a similar binary.
Same sort of response, although here it's less of an issue, but it would be nice if the VAddr was some easy-to-parse value, e.g. 0xFFFFFFFF00000000 or similar.

================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:37
+    VAddr:           0xFFFFFFFF8103398C
+    PAddr:           0x0
+    Offset:          0x1000
----------------
namhyung wrote:
> 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.
Yeah, exactly - it doesn't matter, so let's omit it.

================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:38-41
+    Offset:          0x1000
+    FileSize:        10
+    MemSize:         10
+    Align:           0x1000
----------------
namhyung wrote:
> 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.
Again, same points as above - reduce the noise in the test to make it easy to see what's important.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128705