[PATCH] D69192: llvm-objdump can error out with an unexpected EOF error if .bss is larger than the file being dumped.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

The code change looks good to me.
Comments/suggestions are below.



================
Comment at: llvm/test/tools/llvm-objdump/X86/bss.test:1
+## Check that when BSS is larger than the file llvm-objdump doesn't
+## assert with an unexpected end of file error.
----------------
"bss.test" is probably not a goodname.
I'd suggest "disassemble-bss.test" (it is similar to other disassemble-*.test we have).


================
Comment at: llvm/test/tools/llvm-objdump/X86/bss.test:4
+# RUN: yaml2obj %s | llvm-objdump -D - | FileCheck %s
+# CHECK-NOT: The end of the file was unexpectedly encountered
+--- !ELF
----------------
I'd probably change this. Instead of testing the error message which might change
at any time, you can check that .bss is disassembled:

```
# CHECK: Disassembly of section .bss:
# CHECK: 0000000000000000 .bss:
# CHECK-NEXT:		...
```

I'd also don't optimize the invocations that much and use files:
```
yaml2obj %s -o %t
llvm-objdump -D %t | FileCheck %s
```

It helps to debug tests that fails. With that it is easy to copy paste the
invocation line from the output and have a file created for a futher inspection.


================
Comment at: llvm/test/tools/llvm-objdump/X86/bss.test:5
+# CHECK-NOT: The end of the file was unexpectedly encountered
+--- !ELF
+FileHeader:
----------------
nit: add an empty line between check and yaml.




================
Comment at: llvm/test/tools/llvm-objdump/X86/bss.test:25
+    Binding:         STB_WEAK
+...
----------------
You can simplify to:

```
--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_EXEC
  Machine: EM_X86_64
Sections:
  - Name:  .bss
    Type:  SHT_NOBITS
    Flags: [ SHF_WRITE, SHF_ALLOC ]
    Size:  0x0000000000001000
```


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

https://reviews.llvm.org/D69192