[llvm-objdump][RISCV] Use new common method to parse ARCH RISCV attribute

[eklepilkina]

Summary

Added IgnoreUnknown parameter to parseArchString to allow objdump use this method without failing on unknown extensions

[eklepilkina]

I haven't found any reasons written in comments or docs why llvm-objdump couldn't take the attributes from elf file and decode all instructions without extra flags. Could you please have a look is this change can be valid?

[arichardson]

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

13

IMO objdump should just ignore features it doesn't know about and still decode whatever it knows about. It could still print a warning though.

[kito-cheng]

I've implement this before ( https://reviews.llvm.org/D125204), but I didn't find enough time to complete the error handle part, apparently this patch did better than mine in this part.

The only functional difference is this patch isn't ignore unsupported extension, feel free to take those part into your patch :)

[eklepilkina]

I don't know how I missed this patch. @kito-cheng thanks you, if you aren't against I'll try merge these 2 patches.

[kito-cheng]

if you aren't against I'll try merge these 2 patches.

I am happy to see that happen, just take anything you need from my patch :)

[eklepilkina]

• Ignore unknown extensions

[eklepilkina]

Updated patch, thank you @kito-cheng

[kito-cheng]

llvm/test/MC/RISCV/attribute-with-insts.s

5–8 ↗

(On Diff #481279)

Not sure why this removed? I suppose this should still work as well after this patch?

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

3–4

Those comment can keep now :)

13

And this can drop now.

37–38

Just note why this need change: arch string at least should be canonical order.

[jrtc27]

llvm/lib/Object/ELFObjectFile.cpp

307

s/version/extension/?

310

This means a file that has, say, v0p7, will disassemble as v1p0 rather than being ignored like an extension whose name isn't known?..

llvm/test/tools/llvm-objdump/ELF/RISCV/objdump.s

9 ↗

(On Diff #481279)

Fix

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

6

This isn't true any more

[kito-cheng]

llvm/test/tools/llvm-objdump/ELF/RISCV/extensions.test

1

Failed to apply this patch locally due to missing this file, are you forgot to add a precommit patch for this?

[eklepilkina]

• Review fixes

[eklepilkina]

llvm/lib/Object/ELFObjectFile.cpp

307

It doesn't check version of experimental extensions only. The extension name is checked.

310

If it's experimental extension yes. This can be changed to use more strict rules, but not sure that this is needed for experimental extensions.

llvm/test/tools/llvm-objdump/ELF/RISCV/extensions.test

1

I wasn't told not to load precommit patch at the same review. So yes, it exsits now only on my branch. I can't commit it in upstream, so I have asked a collegue to do this. Precommit patch wil be on master soon.

[eklepilkina]

llvm/test/tools/llvm-objdump/ELF/RISCV/extensions.test

1

Precommit test on main branch https://github.com/llvm/llvm-project/commit/6827934ee0afcf9478f36291b3b5e947fe6d0036

[eklepilkina]

Gently ping

[kito-cheng]

LGTM for RISC-V part, but I would like let @MaskRay to give final approve since this patch also touch something other than RISC-V port.

[kito-cheng]

ping, @MaskRay could you take a look for this, RISC-V part is LGTM, but this patch has modify something more than RISC-V, so I would like to make sure you are happy with those change too :)

[MaskRay]

In D139553#4039569, @kito-cheng wrote:

ping, @MaskRay could you take a look for this, RISC-V part is LGTM, but this patch has modify something more than RISC-V, so I would like to make sure you are happy with those change too :)

Thanks! The intention LGTM to me as well, but the code style and tests need more work.

llvm/include/llvm/MC/SubtargetFeature.h

192

addFeaturesVector

Use ArrayRef if no need to be a vector

llvm/lib/Support/RISCVISAInfo.cpp

595

+= 1 + ConsumeLength

llvm/test/tools/llvm-objdump/ELF/RISCV/objdump.s

1 ↗

(On Diff #481512)

drop < for llvm-mc

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

2014

drop braces

llvm/tools/llvm-profgen/ProfiledBinary.cpp

622

(*...).xxx => ->

[MaskRay]

The addition of IgnoreUnknown should be mentioned in the commit message / Differential summary. And mention who will be the users.

[MaskRay]

llvm/test/tools/llvm-objdump/ELF/RISCV/objdump.s

1 ↗

(On Diff #481512)

objdump.s is not a good name. Add a file-level comment what this test is about.

If you want to test that llvm-objdump by default decodes most instructions, add a few more instructions beside vsetvli (one instruction doesn't tell much).

[eklepilkina]

• Review fixes

[eklepilkina]

• Review fixes

[MaskRay]

Looks great with some nits.

llvm/lib/Object/ELFObjectFile.cpp

305

The common style doesn't add a blank line after a variable declaration.

llvm/test/tools/llvm-objdump/ELF/RISCV/objdump-instr-from-extensions.s

1 ↗

(On Diff #488568)

For binary utilities, new test/tools/llvm-* tests are supposed to use ## for non-RUN-non-CHECK lines.

Nitpicking: objdump- in objdump-instr-from-extensions.s doesn't convey additional value as the test directory has llvm-objdump in the component.
I don't have a particular good name, and hope others can make a suggestion. Perhaps Tag_RISCV_arch.s is sufficiently clear.

llvm-objdump isn't objdump (which usually refers to GNU objdump).

Use the canonical name Tag_RISCV_arch.

24 ↗

(On Diff #488568)

IIUC vfwsub/vfrsub are in the same family and it's too useful to have more than one instructions in one family for the purpose of this test. Perhaps just keep vfsub.vv and delete all lines below (and the blank line above).

[eklepilkina]

• Review fixes

[foad]

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

2014

How does this work? Don't we need to somehow exit after reporting the error? Is there a test for this path?

[eklepilkina]

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

2014

Yep, true, missed this, thank you. I'll make a new review to fix this today. This is already merged.

[foad]

llvm/lib/Support/RISCVISAInfo.cpp

707

This is dead code.

727

This is dead code.

[eklepilkina]

@foad Fixed in https://reviews.llvm.org/D142003

llvm/include/llvm/MC/SubtargetFeature.h

public:
   explicit SubtargetFeatures(StringRef Initial = "");
 
   /// Returns features as a string.
   std::string getString() const;
 
   /// Adds Features.
   void AddFeature(StringRef String, bool Enable = true);
 
+  void addFeaturesVector(const ArrayRef<std::string> OtherFeatures);
+
   /// Returns the vector of individual subtarget features.
   const std::vector<std::string> &getFeatures() const { return Features; }
 
   /// Prints feature string.
   void print(raw_ostream &OS) const;
 
   // Dumps feature info.
   void dump() const;

llvm/include/llvm/Object/COFF.h

public:
   const coff_relocation *getCOFFRelocation(const RelocationRef &Reloc) const;
   unsigned getSectionID(SectionRef Sec) const;
   unsigned getSymbolSectionID(SymbolRef Sym) const;
 
   uint8_t getBytesInAddress() const override;
   StringRef getFileFormatName() const override;
   Triple::ArchType getArch() const override;
   Expected<uint64_t> getStartAddress() const override;
-  SubtargetFeatures getFeatures() const override { return SubtargetFeatures(); }
+  Expected<SubtargetFeatures> getFeatures() const override {
+    return SubtargetFeatures();
+  }
 
   import_directory_iterator import_directory_begin() const;
   import_directory_iterator import_directory_end() const;
   delay_import_directory_iterator delay_import_directory_begin() const;
   delay_import_directory_iterator delay_import_directory_end() const;
   export_directory_iterator export_directory_begin() const;
   export_directory_iterator export_directory_end() const;
   base_reloc_iterator base_reloc_begin() const;

llvm/include/llvm/Object/ELFObjectFile.h

 
 class ELFObjectFileBase : public ObjectFile {
   friend class ELFRelocationRef;
   friend class ELFSectionRef;
   friend class ELFSymbolRef;
 
   SubtargetFeatures getMIPSFeatures() const;
   SubtargetFeatures getARMFeatures() const;
-  SubtargetFeatures getRISCVFeatures() const;
+  Expected<SubtargetFeatures> getRISCVFeatures() const;
   SubtargetFeatures getLoongArchFeatures() const;
 
   StringRef getAMDGPUCPUName() const;
 
 protected:
   ELFObjectFileBase(unsigned int Type, MemoryBufferRef Source);
 
   virtual uint64_t getSymbolSize(DataRefImpl Symb) const = 0;
public:
 
   /// Returns platform-specific object flags, if any.
   virtual unsigned getPlatformFlags() const = 0;
 
   elf_symbol_iterator_range symbols() const;
 
   static bool classof(const Binary *v) { return v->isELF(); }
 
-  SubtargetFeatures getFeatures() const override;
+  Expected<SubtargetFeatures> getFeatures() const override;
 
   std::optional<StringRef> tryGetCPUName() const override;
 
   void setARMSubArch(Triple &TheTriple) const override;
 
   virtual uint16_t getEType() const = 0;
 
   virtual uint16_t getEMachine() const = 0;

llvm/include/llvm/Object/MachO.h

public:
 
   section_iterator section_begin() const override;
   section_iterator section_end() const override;
 
   uint8_t getBytesInAddress() const override;
 
   StringRef getFileFormatName() const override;
   Triple::ArchType getArch() const override;
-  SubtargetFeatures getFeatures() const override { return SubtargetFeatures(); }
+  Expected<SubtargetFeatures> getFeatures() const override {
+    return SubtargetFeatures();
+  }
   Triple getArchTriple(const char **McpuDefault = nullptr) const;
 
   relocation_iterator section_rel_begin(unsigned Index) const;
   relocation_iterator section_rel_end(unsigned Index) const;
 
   dice_iterator begin_dices() const;
   dice_iterator end_dices() const;
 

llvm/include/llvm/Object/ObjectFile.h

public:
   virtual bool hasDebugInfo() const;
 
   /// The number of bytes used to represent an address in this object
   ///        file format.
   virtual uint8_t getBytesInAddress() const = 0;
 
   virtual StringRef getFileFormatName() const = 0;
   virtual Triple::ArchType getArch() const = 0;
-  virtual SubtargetFeatures getFeatures() const = 0;
+  virtual Expected<SubtargetFeatures> getFeatures() const = 0;
   virtual std::optional<StringRef> tryGetCPUName() const {
     return std::nullopt;
   };
   virtual void setARMSubArch(Triple &TheTriple) const { }
   virtual Expected<uint64_t> getStartAddress() const {
     return errorCodeToError(object_error::parse_failed);
   };
 

llvm/include/llvm/Object/Wasm.h

public:
   void getRelocationTypeName(DataRefImpl Rel,
                              SmallVectorImpl<char> &Result) const override;
 
   section_iterator section_begin() const override;
   section_iterator section_end() const override;
   uint8_t getBytesInAddress() const override;
   StringRef getFileFormatName() const override;
   Triple::ArchType getArch() const override;
-  SubtargetFeatures getFeatures() const override;
+  Expected<SubtargetFeatures> getFeatures() const override;
   bool isRelocatableObject() const override;
   bool isSharedObject() const;
 
   struct ReadContext {
     const uint8_t *Start;
     const uint8_t *Ptr;
     const uint8_t *End;
   };

llvm/include/llvm/Object/XCOFFObjectFile.h

public:
   void getRelocationTypeName(DataRefImpl Rel,
                              SmallVectorImpl<char> &Result) const override;
 
   section_iterator section_begin() const override;
   section_iterator section_end() const override;
   uint8_t getBytesInAddress() const override;
   StringRef getFileFormatName() const override;
   Triple::ArchType getArch() const override;
-  SubtargetFeatures getFeatures() const override;
+  Expected<SubtargetFeatures> getFeatures() const override;
   Expected<uint64_t> getStartAddress() const override;
   StringRef mapDebugSectionName(StringRef Name) const override;
   bool isRelocatableObject() const override;
 
   // Below here is the non-inherited interface.
   bool is64Bit() const;
   Expected<StringRef> getRawData(const char *Start, uint64_t Size,
                                  StringRef Name) const;

llvm/include/llvm/Support/RISCVISAInfo.h

typedef std::map<std::string, RISCVExtensionInfo, ExtensionComparator>
       OrderedExtensionMap;
 
   RISCVISAInfo(unsigned XLen, OrderedExtensionMap &Exts)
       : XLen(XLen), FLen(0), MinVLen(0), MaxELen(0), MaxELenFp(0), Exts(Exts) {}
 
   /// Parse RISCV ISA info from arch string.
   static llvm::Expected<std::unique_ptr<RISCVISAInfo>>
   parseArchString(StringRef Arch, bool EnableExperimentalExtension,
-                  bool ExperimentalExtensionVersionCheck = true);
+                  bool ExperimentalExtensionVersionCheck = true,
+                  bool IgnoreUnknown = false);
 
   /// Parse RISCV ISA info from feature vector.
   static llvm::Expected<std::unique_ptr<RISCVISAInfo>>
   parseFeatures(unsigned XLen, const std::vector<std::string> &Features);
 
   /// Convert RISCV ISA info to a feature vector.
   void toFeatures(std::vector<StringRef> &Features,
                   llvm::function_ref<StringRef(const Twine &)> StrAlloc,

llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp

Error LVELFReader::loadTargetInfo(const ObjectFile &Obj) {
   // Detect the architecture from the object file. We usually don't need OS
   // info to lookup a target and create register info.
   Triple TT;
   TT.setArch(Triple::ArchType(Obj.getArch()));
   TT.setVendor(Triple::UnknownVendor);
   TT.setOS(Triple::UnknownOS);
 
   // Features to be passed to target/subtarget
-  SubtargetFeatures Features = Obj.getFeatures();
-
-  return loadGenericTargetInfo(TT.str(), Features.getString());
+  Expected<SubtargetFeatures> Features = Obj.getFeatures();
+  SubtargetFeatures FeaturesValue;
+  if (!Features) {
+    consumeError(Features.takeError());
+    FeaturesValue = SubtargetFeatures();
+  }
+  FeaturesValue = *Features;
+  return loadGenericTargetInfo(TT.str(), FeaturesValue.getString());
 }
 
 void LVELFReader::mapRangeAddress(const ObjectFile &Obj) {
   for (auto Iter = Obj.symbol_begin(); Iter != Obj.symbol_end(); ++Iter) {
     const SymbolRef &Symbol = *Iter;
 
     Expected<SymbolRef::Type> TypeOrErr = Symbol.getType();
     if (!TypeOrErr) {

llvm/lib/MC/SubtargetFeature.cpp

 void SubtargetFeatures::AddFeature(StringRef String, bool Enable) {
   // Don't add empty features.
   if (!String.empty())
     // Convert to lowercase, prepend flag if we don't already have a flag.
     Features.push_back(hasFlag(String) ? String.lower()
                                        : (Enable ? "+" : "-") + String.lower());
 }
 
+void SubtargetFeatures::addFeaturesVector(
+    const ArrayRef<std::string> OtherFeatures) {
+  Features.insert(Features.cend(), OtherFeatures.begin(), OtherFeatures.end());
+}
+
 SubtargetFeatures::SubtargetFeatures(StringRef Initial) {
   // Break up string into separate features
   Split(Features, Initial);
 }
 
 std::string SubtargetFeatures::getString() const {
   return join(Features.begin(), Features.end(), ",");
 }

llvm/lib/Object/ELFObjectFile.cpp

 #include "llvm/Object/ELFTypes.h"
 #include "llvm/Object/Error.h"
 #include "llvm/Support/ARMAttributeParser.h"
 #include "llvm/Support/ARMBuildAttributes.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/RISCVAttributeParser.h"
 #include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 #include <algorithm>
 #include <cstddef>
 #include <cstdint>
 #include <memory>
 #include <optional>
 #include <string>
 #include <utility>
 
case ARMBuildAttrs::AllowDIVExt:
       Features.AddFeature("hwdiv-arm");
       break;
     }
   }
 
   return Features;
 }
 
-SubtargetFeatures ELFObjectFileBase::getRISCVFeatures() const {
+Expected<SubtargetFeatures> ELFObjectFileBase::getRISCVFeatures() const {
   SubtargetFeatures Features;
   unsigned PlatformFlags = getPlatformFlags();
 
   if (PlatformFlags & ELF::EF_RISCV_RVC) {
     Features.AddFeature("c");
   }
 
-  // Add features according to the ELF attribute section.
-  // If there are any unrecognized features, ignore them.
   RISCVAttributeParser Attributes;
   if (Error E = getBuildAttributes(Attributes)) {
-    // TODO Propagate Error.
-    consumeError(std::move(E));
-    return Features; // Keep "c" feature if there is one in PlatformFlags.
+    return E;
   }
 
   std::optional<StringRef> Attr =
       Attributes.getAttributeString(RISCVAttrs::ARCH);
   if (Attr) {
-    // The Arch pattern is [rv32|rv64][i|e]version(_[m|a|f|d|c]version)*
-    // Version string pattern is (major)p(minor). Major and minor are optional.
-    // For example, a version number could be 2p0, 2, or p92.
-    StringRef Arch = *Attr;
-    if (Arch.consume_front("rv32"))
+    // Suppress version checking for experimental extensions to prevent erroring
+    // when getting any unknown version of experimental extension.
+    auto ParseResult = RISCVISAInfo::parseArchString(
+        *Attr, /*EnableExperimentalExtension=*/true,
+        /*ExperimentalExtensionVersionCheck=*/false,
+        /*IgnoreUnknown=*/true);
+    if (!ParseResult)
+      return ParseResult.takeError();
+    auto &ISAInfo = *ParseResult;
+
+    if (ISAInfo->getXLen() == 32)
       Features.AddFeature("64bit", false);
-    else if (Arch.consume_front("rv64"))
+    else if (ISAInfo->getXLen() == 64)
       Features.AddFeature("64bit");
+    else
+      llvm_unreachable("XLEN should be 32 or 64.");
 
-    while (!Arch.empty()) {
-      switch (Arch[0]) {
-      default:
-        break; // Ignore unexpected features.
-      case 'i':
-        Features.AddFeature("e", false);
-        break;
-      case 'd':
-        Features.AddFeature("f"); // D-ext will imply F-ext.
-        [[fallthrough]];
-      case 'e':
-      case 'm':
-      case 'a':
-      case 'f':
-      case 'c':
-        Features.AddFeature(Arch.take_front());
-        break;
-      }
-
-      // FIXME: Handle version numbers.
-      Arch = Arch.drop_until([](char c) { return c == '_' || c == '\0'; });
-      Arch = Arch.drop_while([](char c) { return c == '_'; });
-    }
+    Features.addFeaturesVector(ISAInfo->toFeatureVector());
   }
 
   return Features;
 }
 
 SubtargetFeatures ELFObjectFileBase::getLoongArchFeatures() const {
   SubtargetFeatures Features;
 
   switch (getPlatformFlags() & ELF::EF_LOONGARCH_ABI_MODIFIER_MASK) {
   case ELF::EF_LOONGARCH_ABI_SOFT_FLOAT:
     break;
   case ELF::EF_LOONGARCH_ABI_DOUBLE_FLOAT:
     Features.AddFeature("d");
     // D implies F according to LoongArch ISA spec.
     [[fallthrough]];
   case ELF::EF_LOONGARCH_ABI_SINGLE_FLOAT:
     Features.AddFeature("f");
     break;
   }
 
   return Features;
 }
 
-SubtargetFeatures ELFObjectFileBase::getFeatures() const {
+Expected<SubtargetFeatures> ELFObjectFileBase::getFeatures() const {
   switch (getEMachine()) {
   case ELF::EM_MIPS:
     return getMIPSFeatures();
   case ELF::EM_ARM:
     return getARMFeatures();
   case ELF::EM_RISCV:
     return getRISCVFeatures();
   case ELF::EM_LOONGARCH:

llvm/lib/Object/WasmObjectFile.cpp

 }
 
 StringRef WasmObjectFile::getFileFormatName() const { return "WASM"; }
 
 Triple::ArchType WasmObjectFile::getArch() const {
   return HasMemory64 ? Triple::wasm64 : Triple::wasm32;
 }
 
-SubtargetFeatures WasmObjectFile::getFeatures() const {
+Expected<SubtargetFeatures> WasmObjectFile::getFeatures() const {
   return SubtargetFeatures();
 }
 
 bool WasmObjectFile::isRelocatableObject() const { return HasLinkingSection; }
 
 bool WasmObjectFile::isSharedObject() const { return HasDylinkSection; }
 
 const WasmSection &WasmObjectFile::getWasmSection(DataRefImpl Ref) const {

llvm/lib/Object/XCOFFObjectFile.cpp

 StringRef XCOFFObjectFile::getFileFormatName() const {
   return is64Bit() ? "aix5coff64-rs6000" : "aixcoff-rs6000";
 }
 
 Triple::ArchType XCOFFObjectFile::getArch() const {
   return is64Bit() ? Triple::ppc64 : Triple::ppc;
 }
 
-SubtargetFeatures XCOFFObjectFile::getFeatures() const {
+Expected<SubtargetFeatures> XCOFFObjectFile::getFeatures() const {
   return SubtargetFeatures();
 }
 
 bool XCOFFObjectFile::isRelocatableObject() const {
   if (is64Bit())
     return !(fileHeader64()->Flags & NoRelMask);
   return !(fileHeader32()->Flags & NoRelMask);
 }

llvm/lib/Support/RISCVISAInfo.cpp

else
       ISAInfo->Exts.erase(ExtName.str());
   }
 
   return RISCVISAInfo::postProcessAndChecking(std::move(ISAInfo));
 }
 
 llvm::Expected<std::unique_ptr<RISCVISAInfo>>
 RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
-                              bool ExperimentalExtensionVersionCheck) {
+                              bool ExperimentalExtensionVersionCheck,
+                              bool IgnoreUnknown) {
   // RISC-V ISA strings must be lowercase.
   if (llvm::any_of(Arch, isupper)) {
     return createStringError(errc::invalid_argument,
                              "string must be lowercase");
   }
 
   bool HasRV64 = Arch.startswith("rv64");
   // ISA string must begin with rv32 or rv64.
RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
   // first extension
   Exts = Exts.drop_front(ConsumeLength);
   Exts.consume_front("_");
 
   // TODO: Use version number when setting target features
 
   auto StdExtsItr = StdExts.begin();
   auto StdExtsEnd = StdExts.end();
+  auto GoToNextExt = [](StringRef::iterator &I, unsigned ConsumeLength) {
+    I += 1 + ConsumeLength;
+    if (*I == '_')
+      ++I;
+  };
   for (auto I = Exts.begin(), E = Exts.end(); I != E;) {
     char C = *I;
 
     // Check ISA extensions are specified in the canonical order.
     while (StdExtsItr != StdExtsEnd && *StdExtsItr != C)
       ++StdExtsItr;
 
     if (StdExtsItr == StdExtsEnd) {
for (auto I = Exts.begin(), E = Exts.end(); I != E;) {
     ++StdExtsItr;
 
     std::string Next;
     unsigned Major, Minor, ConsumeLength;
     if (std::next(I) != E)
       Next = std::string(std::next(I), E);
     if (auto E = getExtensionVersion(std::string(1, C), Next, Major, Minor,
                                      ConsumeLength, EnableExperimentalExtension,
-                                     ExperimentalExtensionVersionCheck))
+                                     ExperimentalExtensionVersionCheck)) {
+      if (IgnoreUnknown) {
+        consumeError(std::move(E));
+        GoToNextExt(I, ConsumeLength);
+        continue;
+      }
       return std::move(E);
+    }
 
     // The order is OK, then push it into features.
     // TODO: Use version number when setting target features
     // Currently LLVM supports only "mafdcvh".
-    StringRef SupportedStandardExtension = "mafdcvh";
-    if (!SupportedStandardExtension.contains(C))
+    if (!isSupportedExtension(StringRef(&C, 1))) {
+      if (IgnoreUnknown) {
+        GoToNextExt(I, ConsumeLength);
+        continue;
+      }
       return createStringError(errc::invalid_argument,
                                "unsupported standard user-level extension '%c'",
                                C);
+    }
     ISAInfo->addExtension(std::string(1, C), Major, Minor);
 
     // Consume full extension name and version, including any optional '_'
     // between this extension and the next
-    ++I;
-    I += ConsumeLength;
-    if (*I == '_')
-      ++I;
+    GoToNextExt(I, ConsumeLength);
   }
 
   // Handle other types of extensions other than the standard
   // general purpose and standard user-level extensions.
   // Parse the ISA string containing non-standard user-level
   // extensions, standard supervisor-level extensions and
   // non-standard supervisor-level extensions.
   // These extensions start with 'z', 'x', 's', 'sx' prefixes, follow a
for (StringRef Ext : Split) {
                                  "extension name missing after separator '_'");
 
       StringRef Type = getExtensionType(Ext);
       StringRef Desc = getExtensionTypeDesc(Ext);
       auto Pos = findFirstNonVersionCharacter(Ext) + 1;
       StringRef Name(Ext.substr(0, Pos));
       StringRef Vers(Ext.substr(Pos));
 
-      if (Type.empty())
+      if (Type.empty()) {
+        if (IgnoreUnknown)
+          continue;
         return createStringError(errc::invalid_argument,
                                  "invalid extension prefix '" + Ext + "'");
+      }
 
       // Check ISA extensions are specified in the canonical order.
       while (I != E && *I != Type)
         ++I;
 
-      if (I == E)
+      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 (Name.size() == Type.size()) {
+      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))
+                                       ExperimentalExtensionVersionCheck)) {
+        if (IgnoreUnknown) {
+          consumeError(std::move(E));
+          continue;
+        }
         return std::move(E);
+      }
 
       // Check if duplicated extension.
-      if (llvm::is_contained(AllExts, Name))
+      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/extensions.test

 ## Handle RISCV extensions.
 ## Encode an zbc arch feature into an object file and try to decode it.
 ##
 ## The object file was created with -march=rv32imafdc_zbc
 
 # Fail as unsupported extension is provided
 # RUN: yaml2obj %s -D BITS=32 -o %t.32.o
 # RUN: llvm-objdump -d --no-show-raw-insn %t.32.o \
 # RUN:   | FileCheck %s --check-prefixes=DISASM
 
 # DISASM-LABEL: <clmul>
-# DISASM: <unknown>
+# DISASM: clmul a0, a0, a1
 
 # DISASM-LABEL: <clmulh>
-# DISASM: <unknown>
+# DISASM: clmulh  a0, a0, a1
 
 # DISASM-LABEL: <clmulr>
-# DISASM: <unknown>
+# DISASM: clmulr  a0, a0, a1
 
 --- !ELF
 FileHeader:
   Class:           ELFCLASS32
   Data:            ELFDATA2LSB
   Type:            ET_DYN
   Machine:         EM_RISCV
   Flags:           [ EF_RISCV_RVC, EF_RISCV_FLOAT_ABI_DOUBLE ]
- Length:          0x14
       AddressSize:     0x4
     - Length:          0x1C
       Version:         2
       CuOffset:        0x50E
       AddressSize:     0x4
       Descriptors:
         - Address:         0x506
           Length:          0x5A
 No newline at end of file

llvm/test/tools/llvm-objdump/ELF/RISCV/tag-riscv-arch.s

+## llvm-objdump should decode instructions that are supported by extensions that are used in Tag_RISCV_arch attribute.
+# RUN: llvm-mc -filetype=obj -triple riscv64 %s | \
+# RUN:     llvm-objdump -d -M no-aliases - | \
+# RUN:     FileCheck %s
+  .attribute      5, "rv64gcv"
+# CHECK-LABEL: <foo>:
+foo:
+# CHECK: vsetvli a3, a2, e8, m8, tu, mu
+vsetvli a3, a2, e8, m8, tu, mu
+
+# CHECK: fadd.s fs10, fs11, ft8
+fadd.s f26, f27, f28
+
+# CHECK: fld ft0, 12(a0)
+fld f0, 12(a0)
+
+# CHECK: fmul.d ft0, ft1, ft2, dyn
+fmul.d ft0, ft1, ft2, dyn
+
+# CHECK: vfsub.vv v8, v4, v20, v0.t
+vfsub.vv v8, v4, v20, v0.t
 No newline at end of file

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

 ## Handle unrecognized arch attributes.
 ## Encode an unrecognized arch feature into an object file and try to decode it.
 ## The expected behavior is to ignore the unrecognized arch feature and
 ## continue to process the following arch features.
 ##
-## The object file has the "rv32i2p0_x1p0_m2p0" arch feature. "x1p0" is an
+## The object file has the "rv32i2p0_m2p0_x1p0" arch feature. "x1p0" is an
 ## unrecognized architecture extension. llvm-objdump will ignore it and decode
 ## "mul" instruction correctly according to "m2p0" in the arch feature.
 ##
 ## This test cannot be assembly because the test needs an unrecognized arch
 ## feature and `llvm-mc` will filter out the unrecognized arch feature.
 
 # RUN: yaml2obj %s -D BITS=32 -o %t.32.o
 # RUN: llvm-objdump -d %t.32.o \
 # RUN:   | FileCheck %s --check-prefixes=DISASM
 # RUN: yaml2obj %s -D BITS=64 -o %t.64.o
 # RUN: llvm-objdump -d %t.64.o \
 # RUN:   | FileCheck %s --check-prefixes=DISASM
 
 # DISASM: mul a0, a1, a2
 
 --- !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_x1p0_m2p0"
-    Content: 412300000072697363760001190000000572763332693270305F783170305F6D32703000
+## The content is the encoding of the arch feature "rv32i2p0_m2p0_x1p0"
+    Content: 412300000072697363760001190000000572763332693270305F6D3270305F7831703000

llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp

switch (Analysis.Object->getArch()) {
     case Triple::aarch64:
     case Triple::aarch64_be:
       break;
     default:
       return make_error<UnsupportedDisassembly>("Unsupported architecture.");
   }
 
   Analysis.ObjectTriple = Analysis.Object->makeTriple();
-  Analysis.Features = Analysis.Object->getFeatures();
+  Expected<SubtargetFeatures> Features = Analysis.Object->getFeatures();
+  if (!Features)
+    return Features.takeError();
+
+  Analysis.Features = *Features;
 
   // Init the rest of the object.
   if (auto InitResponse = Analysis.initialiseDisassemblyMembers())
     return std::move(InitResponse);
 
   if (auto SectionParseResponse = Analysis.parseCodeSections())
     return std::move(SectionParseResponse);
 

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

if (std::optional<OwningBinary<Binary>> FetchedBinaryOpt =
         }
       }
     }
   }
 
   const Target *TheTarget = getTarget(Obj);
 
   // Package up features to be passed to target/subtarget
-  SubtargetFeatures Features = Obj->getFeatures();
+  Expected<SubtargetFeatures> FeaturesValue = Obj->getFeatures();
+  if (!FeaturesValue)
+    WithColor::error(errs(), ToolName) << FeaturesValue.takeError();
+  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");
   }
 
   std::unique_ptr<const MCRegisterInfo> MRI(

llvm/tools/llvm-profgen/ProfiledBinary.cpp

void ProfiledBinary::setUpDisassembler(const ELFObjectFileBase *Obj) {
   if (!MRI)
     exitWithError("no register info for target " + TripleName, FileName);
 
   MCTargetOptions MCOptions;
   AsmInfo.reset(TheTarget->createMCAsmInfo(*MRI, TripleName, MCOptions));
   if (!AsmInfo)
     exitWithError("no assembly info for target " + TripleName, FileName);
 
-  SubtargetFeatures Features = Obj->getFeatures();
+  Expected<SubtargetFeatures> Features = Obj->getFeatures();
+  if (!Features)
+    exitWithError(Features.takeError(), FileName);
   STI.reset(
-      TheTarget->createMCSubtargetInfo(TripleName, "", Features.getString()));
+      TheTarget->createMCSubtargetInfo(TripleName, "", Features->getString()));
   if (!STI)
     exitWithError("no subtarget info for target " + TripleName, FileName);
 
   MII.reset(TheTarget->createMCInstrInfo());
   if (!MII)
     exitWithError("no instruction info for target " + TripleName, FileName);
 
   MCContext Ctx(Triple(TripleName), AsmInfo.get(), MRI.get(), STI.get());
   std::unique_ptr<MCObjectFileInfo> MOFI(
       TheTarget->createMCObjectFileInfo(Ctx, /*PIC=*/false));
   Ctx.setObjectFileInfo(MOFI.get());
   DisAsm.reset(TheTarget->createMCDisassembler(*STI, Ctx));