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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/elf-stackmap-version2.test:1
+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-readobj -stackmap %t.o 2>&1 | FileCheck %s
----------------
Please add a comment to the top of this test briefly outlining its purpose.


================
Comment at: llvm/test/tools/llvm-readobj/elf-stackmap-version2.test:2
+# RUN: yaml2obj %s -o %t.o
+# RUN: llvm-readobj -stackmap %t.o 2>&1 | FileCheck %s
+
----------------
We try to use double-dashes for options in new llvm-readobj tests these days, as single-dash causes problems with option grouping.


================
Comment at: llvm/tools/llvm-readobj/StackMapPrinter.h:20
 void prettyPrintStackMap(ScopedPrinter &W, const StackMapParserT &SMP) {
 
+  unsigned version = SMP.getVersion();
----------------
Perhaps unrelated, but please get rid of this blank line.


================
Comment at: llvm/tools/llvm-readobj/StackMapPrinter.h:23
+  if (version != 3) {
+    errs() << "Currently, only supports dumping stack map version 3.\n";
+    return;
----------------
grimar wrote:
> I think James will suggest a different wording, but anyways I think we should include the version found to the error message.
Warnings (and errors) should be lower-case first letter and no trailing full stop or new line, if using reportWarning as @grimar suggests.

Additionally, I'd recommend rewording as:

"stack map entry with version <version> detected at offset <offset>. Only version 3 is supported"

where <version> is the read in version and <offset> is the hex offset of the start of the stack map entry within the section. I'm assuming that there can be multiple entries in a single section, in a linked object (if so, please test this message for multiple such entries).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67328