[PATCH] D92378: [llvm-objdump] Only print unrecognized CPU warning once

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Rather than putting these tests in the tools directory, it probably makes more sense to put it in the `MC` directory, since this is really a test that the library is sane.

Actually, more generally this warning shouldn't be printed directly to `errs()` in the lower-level code, but rather somehow fed back to the tool, so that it can handle it in a tool-specific way. See my lightning talk on "Error Handling in Libraries" from the 2018 developers' meeting for the reasons behind this and some suggested techniques. However, I accept that this might be a wider change than you're anticipating, so happy if you want to ignore it.



================
Comment at: llvm/lib/MC/MCSubtargetInfo.cpp:314
   if (!CPUEntry) {
-    if (CPU != "help") // Don't error if the user asked for help.
-      errs() << "'" << CPU
-             << "' is not a recognized processor for this target"
-             << " (ignoring processor)\n";
+    // A warning for this was already printed in getFeatures()
     return MCSchedModel::GetDefaultSchedModel();
----------------
I know this is the current state of things, but I am slightly hesitant that a future user might not call the `getFeatures()` function for some reason, so the warning will be lost. I don't have a good solution to this at this point though. In llvm-readelf, we have a `reportUniqueWarning` function which only prints a warning that hasn't already been reported. This doesn't exist in other tools to my knowledge however.


================
Comment at: llvm/test/tools/llvm-objdump/warn-mattr-unrecognized.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objdump -d %t --mcpu=alderlake --mattr=not-an-attr 2>&1 | FileCheck %s
----------------
I recommend adding a top-level comment to the test explaining what this test is testing. I've found those useful, even when it seems obvious from the test name.


================
Comment at: llvm/test/tools/llvm-objdump/warn-mattr-unrecognized.test:3
+# RUN: llvm-objdump -d %t --mcpu=alderlake --mattr=not-an-attr 2>&1 | FileCheck %s
+# REQUIRES: x86-registered-target
+
----------------
`REQUIRES` usually go at the start of the file in my experience.


================
Comment at: llvm/test/tools/llvm-objdump/warn-mattr-unrecognized.test:7
+# CHECK-EMPTY:
+## To check we still disassemble the file
+# CHECK-NEXT: file format elf64-x86-64
----------------



================
Comment at: llvm/test/tools/llvm-objdump/warn-mattr-unrecognized.test:12-15
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
----------------
Not really a big issue in this case, but I prefer to not have excessive whitespace in YAML blocks.


================
Comment at: llvm/test/tools/llvm-objdump/warn-mcpu-unrecognized.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objdump -d %t --mcpu=not-a-cpu 2>&1 | FileCheck %s
----------------
All the comments in the other test apply to this one too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92378