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

[Namhyung Kim via Phabricator via llvm-commits]

namhyung marked 5 inline comments as done.
namhyung added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section.test:8
+# RUN: llvm-objdump -d %t | FileCheck %s
+# CHECK:       55                    pushq   %rbp
+# CHECK-NEXT:  48 89 e5              movq    %rsp, %rbp
----------------
jhenderson wrote:
> Sorry, when I said omit the `<>:` entirely, I meant prevent llvm-objdump from producing it at all. If that's not practical (I suspect it isn't for various reasons), I'd suggest leaving it in, so that the check will fail and points out to whoever populates the name where to update a test.
> 
> Also, minor nit, but in the tests I'm usually involved in, we have a single blank line between RUN and CHECK directives, to make it easier to identify the different blocks.
Ok, then I'll leave the part as is and add a blank line.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section.test:15
+# RUN: llvm-objdump -d --start-address=0xffffffff00000000 \
+# RUN: --stop-address=0xffffffff00000004 %t | \
+# RUN: FileCheck %s --check-prefix RANGE
----------------
jhenderson wrote:
> Nit: I usually indent continuation lines a little, as per the inline edit.
Will change.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section.test:16
+# RUN: --stop-address=0xffffffff00000004 %t | \
+# RUN: FileCheck %s --check-prefix RANGE
+# RANGE:       55                    pushq   %rbp
----------------
jhenderson wrote:
> Same as above.
Ok.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section.test:19
+# RANGE-NEXT:  48 89 e5              movq    %rsp, %rbp
+# RANGE-NOT:   0f 1f 40 00           nopl    (%rax)
+
----------------
jhenderson wrote:
> This is a little bit fragile, as it wouldn't be unreasonable for the format of the instruction and operands to change somewhat, which would mean this check would no longer be valid. It might be best simply to do something like `RANGE-EMPTY:` to show that the next line is empty.
Right, will change.


================
Comment at: llvm/test/tools/llvm-objdump/X86/disassemble-no-section.test:41
+    LastSec:         code
+
----------------
jhenderson wrote:
> Nit: too many blank lines at EOF (files should end with exactly one \n).
Will remove.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128705