[llvm-objdump] Fix reporting error for processing target features

[foad]

llvm/lib/Support/RISCVISAInfo.cpp

706–707

Nit: I think the usual style is to keep the braces if the body of the "if" is more than one physical line, even though it is a single statement.

llvm/tools/llvm-objdump/llvm-objdump.cpp

2014

Could be reportError(FeaturesValue.takeError(), Obj->getFileName())?

Is it possible to write a test for this error message?

[eklepilkina]

• Review fixes

[eklepilkina]

• Review fixes

[foad]

LGTM, thanks!

llvm/lib/Support/RISCVISAInfo.cpp

for (StringRef Ext : Split) {
       if (I == E) {
         if (IgnoreUnknown)
           continue;
         return createStringError(errc::invalid_argument,
                                  "%s not given in canonical order '%s'",
                                  Desc.str().c_str(), Ext.str().c_str());
       }
 
       if (!IgnoreUnknown && Name.size() == Type.size()) {
-        if (IgnoreUnknown)
-          continue;
         return createStringError(errc::invalid_argument,
                                  "%s name missing after '%s'",
                                  Desc.str().c_str(), Type.str().c_str());
       }
 
       unsigned Major, Minor, ConsumeLength;
       if (auto E = getExtensionVersion(Name, Vers, Major, Minor, ConsumeLength,
                                        EnableExperimentalExtension,
                                        ExperimentalExtensionVersionCheck)) {
         if (IgnoreUnknown) {
           consumeError(std::move(E));
           continue;
         }
         return std::move(E);
       }
 
       // Check if duplicated extension.
       if (!IgnoreUnknown && llvm::is_contained(AllExts, Name)) {
-        if (IgnoreUnknown)
-          continue;
         return createStringError(errc::invalid_argument, "duplicated %s '%s'",
                                  Desc.str().c_str(), Name.str().c_str());
       }
 
       ISAInfo->addExtension(Name, Major, Minor);
       // Extension format is correct, keep parsing the extensions.
       // TODO: Save Type, Name, Major, Minor to avoid parsing them later.
       AllExts.push_back(Name);

llvm/test/tools/llvm-objdump/ELF/RISCV/invalid-arch-attr.test

+## Handle invalid arch attributes.
+##
+## This test cannot be assembly because the test needs an invalid arch
+## feature and `llvm-mc` will filter out the unrecognized arch feature.
+
+# RUN: yaml2obj %s -D BITS=32 -o %t.32.o
+# RUN: not llvm-objdump -d %t.32.o 2>&1 | \
+# RUN:   FileCheck %s --check-prefixes=DISASM
+
+# DISASM: {{.*}} invalid section length 35 at offset 0x1
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS[[BITS]]
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_RISCV
+Sections:
+  - Name:    .text
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+## The content is the encoding of "mul a0, a1, a2".
+## The encoding could be decoded only when the "m" extension is enabled.
+    Content: 3385C502
+  - Name:    .riscv.attributes
+    Type:    SHT_RISCV_ATTRIBUTES
+## The content is the encoding of the arch feature "rv32i2p0_m2p0_x1p0"
+    Content: 4123000000726973637600019572763332693270305F6D3270305F7831703000

llvm/tools/llvm-objdump/llvm-objdump.cpp

if (Obj->symbols().empty()) {
     }
   }
 
   const Target *TheTarget = getTarget(Obj);
 
   // Package up features to be passed to target/subtarget
   Expected<SubtargetFeatures> FeaturesValue = Obj->getFeatures();
   if (!FeaturesValue)
-    WithColor::error(errs(), ToolName) << FeaturesValue.takeError();
+    reportError(FeaturesValue.takeError(), Obj->getFileName());
   SubtargetFeatures Features = *FeaturesValue;
   if (!MAttrs.empty()) {
     for (unsigned I = 0; I != MAttrs.size(); ++I)
       Features.AddFeature(MAttrs[I]);
   } else if (MCPU.empty() && Obj->getArch() == llvm::Triple::aarch64) {
     Features.AddFeature("+all");
   }