[PATCH] D73531: [llvm-objdump] avoid crash disassembling unknown instruction

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/ARM/unknown-instr.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objdump -D -triple=thumbv8.1m.main-none-eabi -mattr=+mve %t | FileCheck %s
----------------
To be clear, does this test result in a crash without your fix?

It would be a good idea to add a comment to the top of this test to explain what is going on, why you use the switches you do, and why you use the content you do.


================
Comment at: llvm/test/tools/llvm-objdump/ARM/unknown-instr.test:5-6
+# CHECK:        00000000 .text:
+# CHECK-NEXT:       0: fa 33           adds  r3, #250
+# CHECK-NEXT:       2: fe 0c           lsrs  r6, r7, #19
+# CHECK-NEXT:       4: cb              <unknown>
----------------
Are either of these two instructions important for the crash, or can you get rid of them?


================
Comment at: llvm/test/tools/llvm-objdump/ARM/unknown-instr.test:12-15
+  Class:           ELFCLASS32
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_ARM
----------------
In newer tests, we usually try to avoid excessive whitespace as it makes things harder to read. I'd reduce it so there's only a single space between the longest key and it's value, and then line up all the other values as shown:
```
  Class:   ELFCLASS32
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: EM_ARM
```


================
Comment at: llvm/test/tools/llvm-objdump/ARM/unknown-instr.test:17-19
+  - Name: .text
+    Type: SHT_PROGBITS
+    Content: "fa33fe0ccbf3f78bbe"
----------------
Ditto, but this time adding spaces.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1434
 
+        if (!Disassembled) {
+          outs() << "\n";
----------------
jhenderson wrote:
> Let's add a comment here explaining why we don't just let it fall through to the following code. Something like "Continue early, if the disassembler failed to decode the instruction, so that llvm-objdump doesn't try to load other information for that instruction, which may be invalid or incomplete." Happy for alternative suggestions though.
Tip: when you address a comment, click the "Done" checkbox on the comment. This causes the colouring to be obviosuly different. If you do it after you've uploaded your diff, make sure to submit too.


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

https://reviews.llvm.org/D73531