[RISCV] Use RISCVISAInfo to parse arch string from ELF build attribute.

[kito-cheng]

Summary

ELFObjectFileBase::getRISCVFeatures using a handmade simple parser
to parse arch attribute, and that have long time no update, I think the
reasonble way to fix is using RISCVISAInfo, so that we can keep maintain
a unify infrastructure.

And there is an extra requirement for ELFObjectFileBase::getRISCVFeatures,
that might got unrecognized extensions, for example, the object is
compiled by GNU toolchain, and that might contain extensions which is
not supported yet by LLVM.

[liaolucy]

Does this test case need to be updated? llvm/test/MC/RISCV/attribute-with-insts.s

[kito-cheng]

Changes:

• Add missing testcase...

[kito-cheng]

@liaolucy

Does this test case need to be updated? llvm/test/MC/RISCV/attribute-with-insts.s

The testcase already has arch attribute, so assemble disassemble are work fine as past, the difference is disassembler using RISCVISAInfo to parse arch string from ELF attribute.

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

37–38

NOTE: This update the testcase to fit the canonical order.

[liaolucy]

The testcase already has arch attribute, so assemble disassemble are work fine as past, the difference is disassembler using RISCVISAInfo to parse arch string from ELF attribute.

For example, does V extensions（.attribute arch, "xxxx_v1p0"） need to be added to attribute-with-insts.s?

[arichardson]

llvm/include/llvm/Support/RISCVISAInfo.h

49

This name is quite confusing, maybe just IgnoreUnknown?

[kito-cheng]

Changes:

• Rename IgnoreUnknownOrError to IgnoreUnknown.

[kito-cheng]

llvm/include/llvm/Support/RISCVISAInfo.h

49

Thanks!

[jhenderson]

llvm/lib/Object/ELFObjectFile.cpp

308–321

311–312

The final argument needs a comment too.

328–330

What does AddFeature take? Should Feature be a StringRef? If not, I'd still spell out the type rather than use auto.

Also, no need for braces for single-line for loops.

llvm/lib/Support/RISCVISAInfo.cpp

563

I don't know RISC-V at all, but is there any particular reason this is in the order it is? If not, I'd suggest changing it to alphabetical order.

[kito-cheng]

Changes:

• Address @jhenderson's comment
  • Minor style fixing.
  • Refactor RISCVISAInfo::parseArchString, using isSupportedExtension rather than maintain SupportedStandardExtension.

[kito-cheng]

llvm/lib/Object/ELFObjectFile.cpp

328–330

What does AddFeature take? Should Feature be a StringRef?

StringRef, that should take StringRef and take care about the lifetime in my first impression, but I realized that actually store in a std::vector<std::string>[1], so I just pass std::string here.

[1] https://llvm.org/doxygen/SubtargetFeature_8h_source.html#l00184

llvm/lib/Support/RISCVISAInfo.cpp

563

That's part RISC-V standard, we called it canonical order of arch. string[1], but that remind me I should refactor that to using isSupportedExtension rather than maintain a magical string here.

[1] https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L182

[jhenderson]

llvm/lib/Object/ELFObjectFile.cpp

328–330

Since AddFeature takes a StringRef, we should just pass one in, i.e. const std::string & -> StringRef. Lifetime isn't an issue here, so it doesn't matter which we use, but the LLVM guidelines are to use StringRef, I believe.

[kito-cheng]

Changes:

• Address @jhenderson's comment.

[jhenderson]

llvm/lib/Object/ELFObjectFile.cpp

331–332

This is what I meant.

[kito-cheng]

Changes:

• Address @jhenderson's comment, this time should be right :P

[kito-cheng]

llvm/lib/Object/ELFObjectFile.cpp

331–332

I thought range based for should match the type of the source container, it's my first time to know this usage, thanks!

[jhenderson]

No more comments from me, but someone with RISCV knowledge should also review.

[MaskRay]

llvm/lib/Object/ELFObjectFile.cpp

315

This should be fixed.

[kito-cheng]

https://reviews.llvm.org/D139553 is preferred one :)

llvm/include/llvm/Support/RISCVISAInfo.h

public:
   /// OrderedExtensionMap is std::map, it's specialized to keep entries
   /// in canonical order of extension.
   typedef std::map<std::string, RISCVExtensionInfo, ExtensionComparator>
       OrderedExtensionMap;
 
   /// 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 IgnoreUnknownOrError = 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,
                   std::function<StringRef(const Twine &)> StrAlloc) const;

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 <string>
 #include <utility>
 
 using namespace llvm;
SubtargetFeatures ELFObjectFileBase::getRISCVFeatures() const {
   if (Error E = getBuildAttributes(Attributes)) {
     // TODO Propagate Error.
     consumeError(std::move(E));
     return Features; // Keep "c" feature if there is one in PlatformFlags.
   }
 
   Optional<StringRef> Attr = Attributes.getAttributeString(RISCVAttrs::ARCH);
   if (Attr.hasValue()) {
-    // 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.getValue();
-    if (Arch.consume_front("rv32"))
+    // Suppress version checking for experimental extension to prevent that stop
+    // when got any unknown version.
+    auto ParseResult = RISCVISAInfo::parseArchString(
+        *Attr, /* EnableExperimentalExtension */ true,
+        /*ExperimentalExtensionVersionCheck*/ false, true);
+    if (!ParseResult) {
+      // TODO: Propagate Error.
+      consumeError(ParseResult.takeError());
+      return Features;
+    }
+    std::unique_ptr<RISCVISAInfo> &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.
-        LLVM_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 == '_'; });
+    std::vector<std::string> FeatureVector = ISAInfo->toFeatureVector();
+    for (const auto &Feature : FeatureVector) {
+      Features.AddFeature(Feature);
     }
   }
 
   return Features;
 }
 
 SubtargetFeatures ELFObjectFileBase::getFeatures() const {
   switch (getEMachine()) {
   case ELF::EM_MIPS:
     return getMIPSFeatures();
   case ELF::EM_ARM:

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 IgnoreUnknownOrError) {
   // 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();
+  StringRef SupportedStandardExtension = "mafdcbv";
   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 (IgnoreUnknownOrError) {
+        consumeError(std::move(E));
+        goto ParseStdExtFail;
+      }
       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 "mafdcbv".
-    StringRef SupportedStandardExtension = "mafdcbv";
-    if (!SupportedStandardExtension.contains(C))
+    if (!SupportedStandardExtension.contains(C)) {
+      if (IgnoreUnknownOrError)
+        goto ParseStdExtFail;
       return createStringError(errc::invalid_argument,
                                "unsupported standard user-level extension '%c'",
                                C);
+    }
     ISAInfo->addExtension(std::string(1, C), Major, Minor);
 
+  ParseStdExtFail:
     // Consume full extension name and version, including any optional '_'
     // between this extension and the next
     ++I;
     I += ConsumeLength;
     if (*I == '_')
       ++I;
   }
 
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 (IgnoreUnknownOrError)
+          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 (IgnoreUnknownOrError)
+          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 (!IgnoreUnknownOrError && Name.size() == Type.size()) {
+        if (IgnoreUnknownOrError)
+          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 (IgnoreUnknownOrError) {
+          consumeError(std::move(E));
+          continue;
+        }
         return std::move(E);
+      }
 
       // Check if duplicated extension.
-      if (llvm::is_contained(AllExts, Name))
+      if (!IgnoreUnknownOrError && llvm::is_contained(AllExts, Name)) {
+        if (IgnoreUnknownOrError)
+          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/unknown-arch-attr.test

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