[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/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:
> 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).
Ok, I'll update the test as you suggested.


================
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:
> 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?
>>>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.

Yeah, I'd like to split the -h case as there might be more to come later.

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

Then I can update the disassemble test with and without the addresses restrictions.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:12
+
+# DISAS:  <>:
+# DISAS-NEXT:  55                    pushq   %rbp
----------------
jhenderson wrote:
> 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.
I'll omit it for now and add it back when I work on the section name, ok?


================
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:
> 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)?
>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.

Ok, will remove unnecessary bits.

> 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)?

I'm not sure if it worth adding the test now.  In the current implementation, it only creates fake sections if the phdr has PF_X so non-executable phdrs won't work.  When I add proper section headers for all types of phdr, I can add the tests.


================
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:
> 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.
Fair enough, will change.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section-kcore.test:37
+    VAddr:           0xFFFFFFFF8103398C
+    PAddr:           0x0
+    Offset:          0x1000
----------------
jhenderson wrote:
> 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.
Sure.


================
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:
> 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.
Ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128705