[llvm-objdump] Only print unrecognized CPU warning once

[DavidSpickett]

Summary

This would print the warning twice:
$ ./bin/llvm-objdump -d /tmp/test.o --mcpu=not-a-cpu
'not-a-cpu' is not a recognized processor for this target (ignoring processor)
'not-a-cpu' is not a recognized processor for this target (ignoring processor)

/tmp/test.o: file format elf64-x86-64
<...>

This is due to warnings printed from InitMCProcessorInfo.
First it calls getFeatures, which warns for an invalid CPU name.
Then it calls getSchedModelForCPU which warns again for the
same reason.

To fix this, don't print a warning in getSchedModelForCPU.

With one exception, this function is only called from
InitMCProcessorInfo so there's already been a warning printed.

The exception is ARMSubtarget::initSubtargetFeatures.
However, this function first calls ParseSubtargetFeatures,
which always calls InitMCProcessorInfo. So again we will
have already printed a warning by the time we call
getSchedModelForCPU.

This actually affects many llvm tools. llvm-objdump, llvm-mc,
llc, lli, llvm-mca and exegis to my knowledge.

However they are all calling the same code so I'm adding
tests just to llvm-objdump which will catch any regression.

--mattr doesn't have this issue and a test is added to
check this.

[jhenderson]

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.

llvm/lib/MC/MCSubtargetInfo.cpp

314

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.

llvm/test/tools/llvm-objdump/warn-mattr-unrecognized.test

1

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.

3

REQUIRES usually go at the start of the file in my experience.

7

12–15

Not really a big issue in this case, but I prefer to not have excessive whitespace in YAML blocks.

llvm/test/tools/llvm-objdump/warn-mcpu-unrecognized.test

1

All the comments in the other test apply to this one too.

[DavidSpickett]

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

Agreed, I will try that out. Alternatively I might be able to avoid the lone getSchedModel call in the Arm target and make it harder to do things incorrectly.

llvm/lib/MC/MCSubtargetInfo.cpp

 //===- MCSubtargetInfo.cpp - Subtarget Information ------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 

 const MCSchedModel &MCSubtargetInfo::getSchedModelForCPU(StringRef CPU) const {
   assert(llvm::is_sorted(ProcDesc) &&
          "Processor machine model table is not sorted");
 
   // Find entry
   const SubtargetSubTypeKV *CPUEntry = Find(CPU, ProcDesc);
 
   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();
   }
   assert(CPUEntry->SchedModel && "Missing processor SchedModel value");
   return *CPUEntry->SchedModel;
 }
 
 InstrItineraryData
 MCSubtargetInfo::getInstrItineraryForCPU(StringRef CPU) const {

llvm/test/tools/llvm-objdump/warn-mattr-unrecognized.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objdump -d %t --mcpu=alderlake --mattr=not-an-attr 2>&1 | FileCheck %s
+# REQUIRES: x86-registered-target
+
+# CHECK: '+not-an-attr' is not a recognized feature for this target (ignoring feature)
+# CHECK-EMPTY:
+## To check we still disassemble the file
+# CHECK-NEXT: file format elf64-x86-64
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64

llvm/test/tools/llvm-objdump/warn-mcpu-unrecognized.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objdump -d %t --mcpu=not-a-cpu 2>&1 | FileCheck %s
+# REQUIRES: x86-registered-target
+
+# CHECK: 'not-a-cpu' is not a recognized processor for this target (ignoring processor)
+# CHECK-EMPTY:
+## To check we still disassemble the file
+# CHECK-NEXT: file format elf64-x86-64
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64