[llvm] 2c20995 - [RISCV] Detect duplicate extensions in parseNormalizedArchString. (#91416)

[via llvm-commits]

Author: Craig Topper
Date: 2024-05-07T20:56:55-07:00
New Revision: 2c209957819328481554e7c5929d134502b4972a

URL: https://github.com/llvm/llvm-project/commit/2c209957819328481554e7c5929d134502b4972a
DIFF: https://github.com/llvm/llvm-project/commit/2c209957819328481554e7c5929d134502b4972a.diff

LOG: [RISCV] Detect duplicate extensions in parseNormalizedArchString. (#91416)

This detects the same extension name being added twice. Mostly I'm
worried about the case that the same string appears with two different
versions. We will only preserve one of the versions.

We could allow the same version to be repeated, but that doesn't seem
useful at the moment.

I've updated addExtension to use map::emplace instead of
map::operator[]. This means we only keep the first version if there are
duplicates. Previously we kept the last version, but that shouldn't matter
now that we don't allow duplicates. parseArchString already doesn't allow
duplicates.

Added: 
    

Modified: 
    llvm/include/llvm/TargetParser/RISCVISAInfo.h
    llvm/lib/TargetParser/RISCVISAInfo.cpp
    llvm/unittests/TargetParser/RISCVISAInfoTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/TargetParser/RISCVISAInfo.h b/llvm/include/llvm/TargetParser/RISCVISAInfo.h
index 36617a9b62597..12f6b46fb3cee 100644
--- a/llvm/include/llvm/TargetParser/RISCVISAInfo.h
+++ b/llvm/include/llvm/TargetParser/RISCVISAInfo.h
@@ -87,7 +87,7 @@ class RISCVISAInfo {
 
   RISCVISAUtils::OrderedExtensionMap Exts;
 
-  void addExtension(StringRef ExtName, RISCVISAUtils::ExtensionVersion Version);
+  bool addExtension(StringRef ExtName, RISCVISAUtils::ExtensionVersion Version);
 
   Error checkDependency();
 

diff  --git a/llvm/lib/TargetParser/RISCVISAInfo.cpp b/llvm/lib/TargetParser/RISCVISAInfo.cpp
index 9c2ac8c3893f1..96590745b2ebc 100644
--- a/llvm/lib/TargetParser/RISCVISAInfo.cpp
+++ b/llvm/lib/TargetParser/RISCVISAInfo.cpp
@@ -159,9 +159,9 @@ findDefaultVersion(StringRef ExtName) {
   return std::nullopt;
 }
 
-void RISCVISAInfo::addExtension(StringRef ExtName,
+bool RISCVISAInfo::addExtension(StringRef ExtName,
                                 RISCVISAUtils::ExtensionVersion Version) {
-  Exts[ExtName.str()] = Version;
+  return Exts.emplace(ExtName, Version).second;
 }
 
 static StringRef getExtensionTypeDesc(StringRef Ext) {
@@ -492,7 +492,9 @@ RISCVISAInfo::parseNormalizedArchString(StringRef Arch) {
                                "'" + Twine(ExtName[0]) +
                                    "' must be followed by a letter");
 
-    ISAInfo->addExtension(ExtName, {MajorVersion, MinorVersion});
+    if (!ISAInfo->addExtension(ExtName, {MajorVersion, MinorVersion}))
+      return createStringError(errc::invalid_argument,
+                               "duplicate extension '" + ExtName + "'");
   }
   ISAInfo->updateImpliedLengths();
   return std::move(ISAInfo);

diff  --git a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
index 83b52d0527c3a..0e807cfb8e3b8 100644
--- a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
@@ -78,6 +78,14 @@ TEST(ParseNormalizedArchString, RejectsBadX) {
   }
 }
 
+TEST(ParseNormalizedArchString, DuplicateExtension) {
+  for (StringRef Input : {"rv64i2p0_a2p0_a1p0"}) {
+    EXPECT_EQ(
+        toString(RISCVISAInfo::parseNormalizedArchString(Input).takeError()),
+        "duplicate extension 'a'");
+  }
+}
+
 TEST(ParseNormalizedArchString, AcceptsValidBaseISAsAndSetsXLen) {
   auto MaybeRV32I = RISCVISAInfo::parseNormalizedArchString("rv32i2p0");
   ASSERT_THAT_EXPECTED(MaybeRV32I, Succeeded());