[PATCH] D67328: [llvm-readobj] Warn user when dumping not supported version of stack map (PR38278).

Mon Sep 9 01:46:57 PDT 2019

grimar added inline comments.

================
Comment at: llvm/test/tools/llvm-readobj/elf-stackmap-version2.test:11
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
----------------
Please remove the excessive indentations.

================
Comment at: llvm/test/tools/llvm-readobj/elf-stackmap-version2.test:16
+    Flags:           [ SHF_ALLOC ]
+    AddressAlign:    0x0000000000000008
+    Content:         020000000100000003000000010000000000000000000000080000000000000001000000000000000000008000000000FFFFFFFF00000000000000000100000001000000000000000400000000000C000400080000000000FFFFFFFF0400080000000000FFFFFFFF0400080000000000000001000400080000000000009435770400080000000000FFFFFF7F0400080000000000FFFFFFFF0400080000000000FFFFFFFF0400080000000000000000000500080000000000000000000500080000000000010000000500080000000000020000000400080000000000FFFFFFFF0000010007000008
----------------
Doesn't seem you need `AddressAlign` or `Flags` fields?

================
Comment at: llvm/test/tools/llvm-readobj/elf-stackmap-version2.test:17
+    AddressAlign:    0x0000000000000008
+    Content:         020000000100000003000000010000000000000000000000080000000000000001000000000000000000008000000000FFFFFFFF00000000000000000100000001000000000000000400000000000C000400080000000000FFFFFFFF0400080000000000FFFFFFFF0400080000000000000001000400080000000000009435770400080000000000FFFFFF7F0400080000000000FFFFFFFF0400080000000000FFFFFFFF0400080000000000000000000500080000000000000000000500080000000000010000000500080000000000020000000400080000000000FFFFFFFF0000010007000008
+...
----------------
Would be nice to have a comment about how you got this content.
However, it seems it can be just `Content: 02` for now?

================
Comment at: llvm/tools/llvm-readobj/StackMapPrinter.h:21

+  unsigned version = SMP.getVersion();
+  if (version != 3) {
----------------
Variable name should be uppercase.

================
Comment at: llvm/tools/llvm-readobj/StackMapPrinter.h:23
+  if (version != 3) {
+    errs() << "Currently, only supports dumping stack map version 3.\n";
+    return;
----------------
I think James will suggest a different wording, but anyways I think we should include the version found to the error message.

================
Comment at: llvm/tools/llvm-readobj/StackMapPrinter.h:24
+    errs() << "Currently, only supports dumping stack map version 3.\n";
+    return;
+  }
----------------
You should use `reportWarning` from llvm-readobj.h

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67328