[lld][RISCV] Avoid error when encountering unrecognised ISA extensions/versions in RISC-V attributes

This patch follows on from this RFC thread
<https://discourse.llvm.org/t/rfc-resolving-issues-related-to-extension-versioning-in-risc-v/68472/>
and the ensuing discussion in the RISC-V LLVM sync-up call. The
consensus agreed that the behaviour change in LLD introduced in D138550
that results in object files including arch attributes with unrecognised
extensions or versions of extensions is a regression and should be
treated as such. As it stands, this logic means that LLD will error out
if trying to link a RISC-V object file from LLVM HEAD (rv32i2p0/rv64i2p0)
with one from current GCC (rv32i2p1/rv64i2p1 by default).

There's a bigger discussion about exactly when to warn vs error and so
on, and how to control that, and this patch doesn't attempt to address
all those questions. It simply tries to fix the problem with a minimally
invasive change, intended to be cherry-picked for 16.0.x (ideally
16.0.0, but queued for 16.0.1 if we're too late in the release cycle).

As you can see from the test changes, although the changed logic is
mostly more permissive, it will reject some embedded arch strings that
were accepted before. Because the same logic was previously used for
parsing human-written -march as for the arch attribute (intended to be
stored in normalised form), various short-hand formats were previously
accepted. Maintaining support for such ill-formed attributes would
substantially complicate the logic, and given the previous
implementation was so much stricter in every other way, was surely a bug
rather than a design choice.

Surprisingly, the precise rules for how numbers can be embedded into
extension names isn't fully defined (there is a PR to the ISA manual
that is not yet merged
<riscv/riscv-isa-manual#718>).
In the absence of specific criteria for rejecting extensions names that
would be ambiguous in abbreviated form,
`RISCVISAInfo::parseArchStringNormalized` just pulls out the version
information from the end of each extension description.

Differential Revision: https://reviews.llvm.org/D144353

Author: asb

lld/ELF/Arch/RISCV.cpp

@@ -848,9 +848,7 @@ class RISCVAttributesSection final : public SyntheticSection {
 static void mergeArch(RISCVISAInfo::OrderedExtensionMap &mergedExts,
                       unsigned &mergedXlen, const InputSectionBase *sec,
                       StringRef s) {
-  auto maybeInfo =
-      RISCVISAInfo::parseArchString(s, /*EnableExperimentalExtension=*/true,
-                                    /*ExperimentalExtensionVersionCheck=*/true);
+  auto maybeInfo = RISCVISAInfo::parseNormalizedArchString(s);
   if (!maybeInfo) {
     errorOrWarn(toString(sec) + ": " + s + ": " +
                 llvm::toString(maybeInfo.takeError()));
@@ -865,8 +863,6 @@ static void mergeArch(RISCVISAInfo::OrderedExtensionMap &mergedExts,
   } else {
     for (const auto &ext : info.getExtensions()) {
       if (auto it = mergedExts.find(ext.first); it != mergedExts.end()) {
-        // TODO This is untested because RISCVISAInfo::parseArchString does not
-        // accept unsupported versions yet.
         if (std::tie(it->second.MajorVersion, it->second.MinorVersion) >=
             std::tie(ext.second.MajorVersion, ext.second.MinorVersion))
           continue;

lld/test/ELF/riscv-attributes.s

@@ -15,20 +15,24 @@
 # RUN: llvm-readobj --arch-specific out2 | FileCheck %s --check-prefix=CHECK2
 
 # RUN: llvm-mc -filetype=obj -triple=riscv64 unrecognized_ext1.s -o unrecognized_ext1.o
-# RUN: not ld.lld unrecognized_ext1.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=UNRECOGNIZED_EXT1 --implicit-check-not=error:
-# UNRECOGNIZED_EXT1: error: unrecognized_ext1.o:(.riscv.attributes): rv64i2p0_y2p0: invalid standard user-level extension 'y'
+# RUN: ld.lld -e 0 unrecognized_ext1.o -o unrecognized_ext1 2>&1 | count 0
+# RUN: llvm-readobj --arch-specific unrecognized_ext1 | FileCheck %s --check-prefix=UNRECOGNIZED_EXT1
 
 # RUN: llvm-mc -filetype=obj -triple=riscv64 unrecognized_ext2.s -o unrecognized_ext2.o
-# RUN: not ld.lld unrecognized_ext2.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=UNRECOGNIZED_EXT2 --implicit-check-not=error:
-# UNRECOGNIZED_EXT2: error: unrecognized_ext2.o:(.riscv.attributes): rv64i2p0_zmadeup1p0: unsupported version number 1.0 for extension 'zmadeup'
+# RUN: ld.lld -e 0 unrecognized_ext2.o -o unrecognized_ext2 2>&1 | count 0
+# RUN: llvm-readobj --arch-specific unrecognized_ext2 | FileCheck %s --check-prefix=UNRECOGNIZED_EXT2
 
 # RUN: llvm-mc -filetype=obj -triple=riscv64 unrecognized_version.s -o unrecognized_version.o
-# RUN: not ld.lld unrecognized_version.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=UNRECOGNIZED_VERSION --implicit-check-not=error:
-# UNRECOGNIZED_VERSION: error: unrecognized_version.o:(.riscv.attributes): rv64i99p0: unsupported version number 99.0 for extension 'i'
+# RUN: ld.lld -e 0 unrecognized_version.o -o unrecognized_version 2>&1 | count 0
+# RUN: llvm-readobj --arch-specific unrecognized_version | FileCheck %s --check-prefix=UNRECOGNIZED_VERSION
+
+# RUN: llvm-mc -filetype=obj -triple=riscv64 merge_version_test_input.s -o merge_version_test_input.o
+# RUN: ld.lld -e 0 unrecognized_version.o merge_version_test_input.o -o out3 2>&1 | count 0
+# RUN: llvm-readobj --arch-specific out3 | FileCheck %s --check-prefix=CHECK3
 
 # RUN: llvm-mc -filetype=obj -triple=riscv64 invalid_arch1.s -o invalid_arch1.o
-# RUN: ld.lld -e 0 invalid_arch1.o -o invalid_arch1
-# RUN: llvm-readobj --arch-specific invalid_arch1 | FileCheck %s --check-prefix=INVALID_ARCH1
+# RUN: not ld.lld invalid_arch1.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=INVALID_ARCH1 --implicit-check-not=error:
+# INVALID_ARCH1: error: invalid_arch1.o:(.riscv.attributes): rv64i2: extension lacks version in expected format
 
 ## A zero value attribute is not printed.
 # RUN: llvm-mc -filetype=obj -triple=riscv64 unaligned_access_0.s -o unaligned_access_0.o
@@ -121,6 +125,23 @@
 # CHECK2-NEXT:   }
 # CHECK2-NEXT: }
 
+# CHECK3:      BuildAttributes {
+# CHECK3-NEXT:   FormatVersion: 0x41
+# CHECK3-NEXT:   Section 1 {
+# CHECK3-NEXT:     SectionLength: 26
+# CHECK3-NEXT:     Vendor: riscv
+# CHECK3-NEXT:     Tag: Tag_File (0x1)
+# CHECK3-NEXT:     Size: 16
+# CHECK3-NEXT:     FileAttributes {
+# CHECK3-NEXT:       Attribute {
+# CHECK3-NEXT:         Tag: 5
+# CHECK3-NEXT:         TagName: arch
+# CHECK3-NEXT:         Value: rv64i99p0
+# CHECK3-NEXT:       }
+# CHECK3-NEXT:     }
+# CHECK3-NEXT:   }
+# CHECK3-NEXT: }
+
 #--- a.s
 .attribute stack_align, 16
 .attribute arch, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"
@@ -140,6 +161,22 @@
 .attribute priv_spec_minor, 2
 
 #--- unrecognized_ext1.s
+# UNRECOGNIZED_EXT1:      BuildAttributes {
+# UNRECOGNIZED_EXT1-NEXT:   FormatVersion: 0x41
+# UNRECOGNIZED_EXT1-NEXT:   Section 1 {
+# UNRECOGNIZED_EXT1-NEXT:     SectionLength: 30
+# UNRECOGNIZED_EXT1-NEXT:     Vendor: riscv
+# UNRECOGNIZED_EXT1-NEXT:     Tag: Tag_File (0x1)
+# UNRECOGNIZED_EXT1-NEXT:     Size: 20
+# UNRECOGNIZED_EXT1-NEXT:     FileAttributes {
+# UNRECOGNIZED_EXT1-NEXT:       Attribute {
+# UNRECOGNIZED_EXT1-NEXT:         Tag: 5
+# UNRECOGNIZED_EXT1-NEXT:         TagName: arch
+# UNRECOGNIZED_EXT1-NEXT:         Value: rv64i2p0_y2p0
+# UNRECOGNIZED_EXT1-NEXT:       }
+# UNRECOGNIZED_EXT1-NEXT:     }
+# UNRECOGNIZED_EXT1-NEXT:   }
+# UNRECOGNIZED_EXT1-NEXT: }
 .section .riscv.attributes,"",@0x70000003
 .byte 0x41
 .long .Lend-.riscv.attributes-1
@@ -152,6 +189,22 @@
 .Lend:
 
 #--- unrecognized_ext2.s
+# UNRECOGNIZED_EXT2:      BuildAttributes {
+# UNRECOGNIZED_EXT2-NEXT:   FormatVersion: 0x41
+# UNRECOGNIZED_EXT2-NEXT:   Section 1 {
+# UNRECOGNIZED_EXT2-NEXT:     SectionLength: 36
+# UNRECOGNIZED_EXT2-NEXT:     Vendor: riscv
+# UNRECOGNIZED_EXT2-NEXT:     Tag: Tag_File (0x1)
+# UNRECOGNIZED_EXT2-NEXT:     Size: 26
+# UNRECOGNIZED_EXT2-NEXT:     FileAttributes {
+# UNRECOGNIZED_EXT2-NEXT:       Attribute {
+# UNRECOGNIZED_EXT2-NEXT:         Tag: 5
+# UNRECOGNIZED_EXT2-NEXT:         TagName: arch
+# UNRECOGNIZED_EXT2-NEXT:         Value: rv64i2p0_zmadeup1p0
+# UNRECOGNIZED_EXT2-NEXT:       }
+# UNRECOGNIZED_EXT2-NEXT:     }
+# UNRECOGNIZED_EXT2-NEXT:   }
+# UNRECOGNIZED_EXT2-NEXT: }
 .section .riscv.attributes,"",@0x70000003
 .byte 0x41
 .long .Lend-.riscv.attributes-1
@@ -164,6 +217,22 @@
 .Lend:
 
 #--- unrecognized_version.s
+# UNRECOGNIZED_VERSION:      BuildAttributes {
+# UNRECOGNIZED_VERSION-NEXT:   FormatVersion: 0x41
+# UNRECOGNIZED_VERSION-NEXT:   Section 1 {
+# UNRECOGNIZED_VERSION-NEXT:     SectionLength: 26
+# UNRECOGNIZED_VERSION-NEXT:     Vendor: riscv
+# UNRECOGNIZED_VERSION-NEXT:     Tag: Tag_File (0x1)
+# UNRECOGNIZED_VERSION-NEXT:     Size: 16
+# UNRECOGNIZED_VERSION-NEXT:     FileAttributes {
+# UNRECOGNIZED_VERSION-NEXT:       Attribute {
+# UNRECOGNIZED_VERSION-NEXT:         Tag: 5
+# UNRECOGNIZED_VERSION-NEXT:         TagName: arch
+# UNRECOGNIZED_VERSION-NEXT:         Value: rv64i99p0
+# UNRECOGNIZED_VERSION-NEXT:       }
+# UNRECOGNIZED_VERSION-NEXT:     }
+# UNRECOGNIZED_VERSION-NEXT:   }
+# UNRECOGNIZED_VERSION-NEXT: }
 .section .riscv.attributes,"",@0x70000003
 .byte 0x41
 .long .Lend-.riscv.attributes-1
@@ -175,23 +244,19 @@
 .asciz "rv64i99p0"
 .Lend:
 
+#--- merge_version_test_input.s
+.section .riscv.attributes,"",@0x70000003
+.byte 0x41
+.long .Lend-.riscv.attributes-1
+.asciz "riscv"  # vendor
+.Lbegin:
+.byte 1  # Tag_File
+.long .Lend-.Lbegin
+.byte 5  # Tag_RISCV_arch
+.asciz "rv64i2p1"
+.Lend:
+
 #--- invalid_arch1.s
-# INVALID_ARCH1:      BuildAttributes {
-# INVALID_ARCH1-NEXT:   FormatVersion: 0x41
-# INVALID_ARCH1-NEXT:   Section 1 {
-# INVALID_ARCH1-NEXT:     SectionLength: 25
-# INVALID_ARCH1-NEXT:     Vendor: riscv
-# INVALID_ARCH1-NEXT:     Tag: Tag_File (0x1)
-# INVALID_ARCH1-NEXT:     Size: 15
-# INVALID_ARCH1-NEXT:     FileAttributes {
-# INVALID_ARCH1-NEXT:       Attribute {
-# INVALID_ARCH1-NEXT:         Tag: 5
-# INVALID_ARCH1-NEXT:         TagName: arch
-# INVALID_ARCH1-NEXT:         Value: rv64i2p0
-# INVALID_ARCH1-NEXT:       }
-# INVALID_ARCH1-NEXT:     }
-# INVALID_ARCH1-NEXT:   }
-# INVALID_ARCH1-NEXT: }
 .section .riscv.attributes,"",@0x70000003
 .byte 0x41
 .long .Lend-.riscv.attributes-1

llvm/include/llvm/Support/RISCVISAInfo.h

@@ -51,6 +51,12 @@ class RISCVISAInfo {
                   bool ExperimentalExtensionVersionCheck = true,
                   bool IgnoreUnknown = false);
 
+  /// Parse RISCV ISA info from an arch string that is already in normalized
+  /// form (as defined in the psABI). Unlike parseArchString, this function
+  /// will not error for unrecognized extension names or extension versions.
+  static llvm::Expected<std::unique_ptr<RISCVISAInfo>>
+  parseNormalizedArchString(StringRef Arch);
+
   /// Parse RISCV ISA info from feature vector.
   static llvm::Expected<std::unique_ptr<RISCVISAInfo>>
   parseFeatures(unsigned XLen, const std::vector<std::string> &Features);

llvm/lib/Support/RISCVISAInfo.cpp

@@ -519,6 +519,67 @@ RISCVISAInfo::parseFeatures(unsigned XLen,
   return RISCVISAInfo::postProcessAndChecking(std::move(ISAInfo));
 }
 
+llvm::Expected<std::unique_ptr<RISCVISAInfo>>
+RISCVISAInfo::parseNormalizedArchString(StringRef Arch) {
+  if (llvm::any_of(Arch, isupper)) {
+    return createStringError(errc::invalid_argument,
+                             "string must be lowercase");
+  }
+  // Must start with a valid base ISA name.
+  unsigned XLen;
+  if (Arch.startswith("rv32i") || Arch.startswith("rv32e"))
+    XLen = 32;
+  else if (Arch.startswith("rv64i") || Arch.startswith("rv64e"))
+    XLen = 64;
+  else
+    return createStringError(errc::invalid_argument,
+                             "arch string must begin with valid base ISA");
+  std::unique_ptr<RISCVISAInfo> ISAInfo(new RISCVISAInfo(XLen));
+  // Discard rv32/rv64 prefix.
+  Arch = Arch.substr(4);
+
+  // Each extension is of the form ${name}${major_version}p${minor_version}
+  // and separated by _. Split by _ and then extract the name and version
+  // information for each extension.
+  SmallVector<StringRef, 8> Split;
+  Arch.split(Split, '_');
+  for (StringRef Ext : Split) {
+    StringRef Prefix, MinorVersionStr;
+    std::tie(Prefix, MinorVersionStr) = Ext.rsplit('p');
+    if (MinorVersionStr.empty())
+      return createStringError(errc::invalid_argument,
+                               "extension lacks version in expected format");
+    unsigned MajorVersion, MinorVersion;
+    if (MinorVersionStr.getAsInteger(10, MinorVersion))
+      return createStringError(errc::invalid_argument,
+                               "failed to parse minor version number");
+
+    // Split Prefix into the extension name and the major version number
+    // (the trailing digits of Prefix).
+    int TrailingDigits = 0;
+    StringRef ExtName = Prefix;
+    while (!ExtName.empty()) {
+      if (!isDigit(ExtName.back()))
+        break;
+      ExtName = ExtName.drop_back(1);
+      TrailingDigits++;
+    }
+    if (!TrailingDigits)
+      return createStringError(errc::invalid_argument,
+                               "extension lacks version in expected format");
+
+    StringRef MajorVersionStr = Prefix.take_back(TrailingDigits);
+    if (MajorVersionStr.getAsInteger(10, MajorVersion))
+      return createStringError(errc::invalid_argument,
+                               "failed to parse major version number");
+    ISAInfo->addExtension(ExtName, MajorVersion, MinorVersion);
+  }
+  ISAInfo->updateFLen();
+  ISAInfo->updateMinVLen();
+  ISAInfo->updateMaxELen();
+  return std::move(ISAInfo);
+}
+
 llvm::Expected<std::unique_ptr<RISCVISAInfo>>
 RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
                               bool ExperimentalExtensionVersionCheck,

llvm/unittests/Support/CMakeLists.txt

@@ -68,6 +68,7 @@ add_llvm_unittest(SupportTests
   ReverseIterationTest.cpp
   ReplaceFileTest.cpp
   RISCVAttributeParserTest.cpp
+  RISCVISAInfoTest.cpp
   ScaledNumberTest.cpp
   ScopedPrinterTest.cpp
   SHA256.cpp

llvm/unittests/Support/RISCVISAInfoTest.cpp

@@ -0,0 +1,105 @@
+//===-- unittests/RISCVISAInfoTest.cpp ------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/RISCVISAInfo.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+bool operator==(const llvm::RISCVExtensionInfo &A,
+                const llvm::RISCVExtensionInfo &B) {
+  return A.ExtName == B.ExtName && A.MajorVersion == B.MajorVersion &&
+         A.MinorVersion == B.MinorVersion;
+}
+
+TEST(ParseNormalizedArchString, RejectsUpperCase) {
+  for (StringRef Input : {"RV32", "rV64", "rv32i2P0", "rv64i2p0_A2p0"}) {
+    EXPECT_EQ(
+        toString(RISCVISAInfo::parseNormalizedArchString(Input).takeError()),
+        "string must be lowercase");
+  }
+}
+
+TEST(ParseNormalizedArchString, RejectsInvalidBaseISA) {
+  for (StringRef Input : {"rv32", "rv64", "rv32j", "rv65i"}) {
+    EXPECT_EQ(
+        toString(RISCVISAInfo::parseNormalizedArchString(Input).takeError()),
+        "arch string must begin with valid base ISA");
+  }
+}
+
+TEST(ParseNormalizedArchString, RejectsMalformedInputs) {
+  for (StringRef Input : {"rv64i2p0_", "rv32i2p0__a2p0", "rv32e2.0", "rv64e2p",
+                          "rv32i", "rv64ip1"}) {
+    EXPECT_EQ(
+        toString(RISCVISAInfo::parseNormalizedArchString(Input).takeError()),
+        "extension lacks version in expected format");
+  }
+}
+
+TEST(ParseNormalizedArchString, AcceptsValidBaseISAsAndSetsXLen) {
+  auto MaybeRV32I = RISCVISAInfo::parseNormalizedArchString("rv32i2p0");
+  ASSERT_THAT_EXPECTED(MaybeRV32I, Succeeded());
+  RISCVISAInfo &InfoRV32I = **MaybeRV32I;
+  EXPECT_EQ(InfoRV32I.getExtensions().size(), 1UL);
+  EXPECT_TRUE(InfoRV32I.getExtensions().at("i") ==
+              (RISCVExtensionInfo{"i", 2, 0}));
+  EXPECT_EQ(InfoRV32I.getXLen(), 32U);
+
+  auto MaybeRV32E = RISCVISAInfo::parseNormalizedArchString("rv32e2p0");
+  ASSERT_THAT_EXPECTED(MaybeRV32E, Succeeded());
+  RISCVISAInfo &InfoRV32E = **MaybeRV32E;
+  EXPECT_EQ(InfoRV32E.getExtensions().size(), 1UL);
+  EXPECT_TRUE(InfoRV32E.getExtensions().at("e") ==
+              (RISCVExtensionInfo{"e", 2, 0}));
+  EXPECT_EQ(InfoRV32I.getXLen(), 32U);
+
+  auto MaybeRV64I = RISCVISAInfo::parseNormalizedArchString("rv64i2p0");
+  ASSERT_THAT_EXPECTED(MaybeRV64I, Succeeded());
+  RISCVISAInfo &InfoRV64I = **MaybeRV64I;
+  EXPECT_EQ(InfoRV64I.getExtensions().size(), 1UL);
+  EXPECT_TRUE(InfoRV64I.getExtensions().at("i") ==
+              (RISCVExtensionInfo{"i", 2, 0}));
+  EXPECT_EQ(InfoRV64I.getXLen(), 64U);
+
+  auto MaybeRV64E = RISCVISAInfo::parseNormalizedArchString("rv64e2p0");
+  ASSERT_THAT_EXPECTED(MaybeRV64E, Succeeded());
+  RISCVISAInfo &InfoRV64E = **MaybeRV64E;
+  EXPECT_EQ(InfoRV64E.getExtensions().size(), 1UL);
+  EXPECT_TRUE(InfoRV64E.getExtensions().at("e") ==
+              (RISCVExtensionInfo{"e", 2, 0}));
+  EXPECT_EQ(InfoRV64I.getXLen(), 64U);
+}
+
+TEST(ParseNormalizedArchString, AcceptsArbitraryExtensionsAndVersions) {
+  auto MaybeISAInfo = RISCVISAInfo::parseNormalizedArchString(
+      "rv64i5p1_m3p2_zmadeup11p12_sfoo2p0_xbar3p0");
+  ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
+  RISCVISAInfo &Info = **MaybeISAInfo;
+  EXPECT_EQ(Info.getExtensions().size(), 5UL);
+  EXPECT_TRUE(Info.getExtensions().at("i") == (RISCVExtensionInfo{"i", 5, 1}));
+  EXPECT_TRUE(Info.getExtensions().at("m") == (RISCVExtensionInfo{"m", 3, 2}));
+  EXPECT_TRUE(Info.getExtensions().at("zmadeup") ==
+              (RISCVExtensionInfo{"zmadeup", 11, 12}));
+  EXPECT_TRUE(Info.getExtensions().at("sfoo") ==
+              (RISCVExtensionInfo{"sfoo", 2, 0}));
+  EXPECT_TRUE(Info.getExtensions().at("xbar") ==
+              (RISCVExtensionInfo{"xbar", 3, 0}));
+}
+
+TEST(ParseNormalizedArchString, UpdatesFLenMinVLenMaxELen) {
+  auto MaybeISAInfo = RISCVISAInfo::parseNormalizedArchString(
+      "rv64i2p0_d2p0_zvl64b1p0_zve64d1p0");
+  ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
+  RISCVISAInfo &Info = **MaybeISAInfo;
+  EXPECT_EQ(Info.getXLen(), 64U);
+  EXPECT_EQ(Info.getFLen(), 64U);
+  EXPECT_EQ(Info.getMinVLen(), 64U);
+  EXPECT_EQ(Info.getMaxELen(), 64U);
+}