[PATCH] D67328: [llvm-readobj] Warn user when dumping not supported version of stack map (PR38278).
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 9 04:05:02 PDT 2019
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
More information about the llvm-commits
mailing list