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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Your patch (probably) is causing llvm/test/Object/objdump-no-sectionheaders.test to fail. Please fix!



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


================
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
----------------
Nit: I usually indent continuation lines a little, as per the inline edit.


================
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
----------------
Same as above.


================
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)
+
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128705