[clang] 55c0048 - [Clang][Driver] Skip empty strings in getAArch64MultilibFlags (#97827)

[via cfe-commits]

Author: Simon Tatham
Date: 2024-07-11T10:28:05+01:00
New Revision: 55c00485eb05dfa75d4a20efff2131edd111066d

URL: https://github.com/llvm/llvm-project/commit/55c00485eb05dfa75d4a20efff2131edd111066d
DIFF: https://github.com/llvm/llvm-project/commit/55c00485eb05dfa75d4a20efff2131edd111066d.diff

LOG: [Clang][Driver] Skip empty strings in getAArch64MultilibFlags (#97827)

In a multilib setting, if you compile with a command line such as `clang
--target=aarch64-none-elf -march=armv8.9-a+rcpc3`,
`getAArch64MultilibFlags` returns an ill-formed string containing two
consecutive `+` signs, of the form `...+rcpc++rcpc3+...`, causing later
stages of multilib selection to get confused.

The `++` arises from the entry in `AArch64::Extensions` for the
SubtargetFeature `rcpc-immo`, which is a dependency of the `rcpc3`
SubtargetFeature, but doesn't have an _extension_ name for the purposes
of the `-march=foo+bar` option. So its `UserVisibleName` field is the
empty string.

To fix this, I've excluded extensions from consideration in
`getAArch64MultilibFlags` if they have an empty `UserVisibleName`. Since
the input to this function is not derived from a completely general set
of SubtargetFeatures, but from a set that has only just been converted
_from_ a clang driver command line, the only extensions skipped by this
check should be cases like this one, where the anonymous extension was
only included because it was a dependency of one mentioned explicitly.

I've also made the analogous change in `getARMMultilibFlags`. I don't
think it's necessary right now, because the architecture extensions for
ARM (defined in `ARMTargetParser.def` rather than Tablegen) don't
include any anonymous ones. But it seems sensible to add the check
anyway, in case future refactoring introduces anonymous array elements
in the same way that AArch64 did, and also in case someone writes a
function for another platform by using either of these as example code.

Added: 
    clang/test/Driver/aarch64-multilib-rcpc3.c

Modified: 
    clang/lib/Driver/ToolChain.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 977e08390800d..85ae4d2a26fee 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -195,11 +195,13 @@ static void getAArch64MultilibFlags(const Driver &D,
                                        UnifiedFeatures.end());
   std::vector<std::string> MArch;
   for (const auto &Ext : AArch64::Extensions)
-    if (FeatureSet.contains(Ext.PosTargetFeature))
-      MArch.push_back(Ext.UserVisibleName.str());
+    if (!Ext.UserVisibleName.empty())
+      if (FeatureSet.contains(Ext.PosTargetFeature))
+        MArch.push_back(Ext.UserVisibleName.str());
   for (const auto &Ext : AArch64::Extensions)
-    if (FeatureSet.contains(Ext.NegTargetFeature))
-      MArch.push_back(("no" + Ext.UserVisibleName).str());
+    if (!Ext.UserVisibleName.empty())
+      if (FeatureSet.contains(Ext.NegTargetFeature))
+        MArch.push_back(("no" + Ext.UserVisibleName).str());
   StringRef ArchName;
   for (const auto &ArchInfo : AArch64::ArchInfos)
     if (FeatureSet.contains(ArchInfo->ArchFeature))
@@ -221,11 +223,13 @@ static void getARMMultilibFlags(const Driver &D,
                                        UnifiedFeatures.end());
   std::vector<std::string> MArch;
   for (const auto &Ext : ARM::ARCHExtNames)
-    if (FeatureSet.contains(Ext.Feature))
-      MArch.push_back(Ext.Name.str());
+    if (!Ext.Name.empty())
+      if (FeatureSet.contains(Ext.Feature))
+        MArch.push_back(Ext.Name.str());
   for (const auto &Ext : ARM::ARCHExtNames)
-    if (FeatureSet.contains(Ext.NegFeature))
-      MArch.push_back(("no" + Ext.Name).str());
+    if (!Ext.Name.empty())
+      if (FeatureSet.contains(Ext.NegFeature))
+        MArch.push_back(("no" + Ext.Name).str());
   MArch.insert(MArch.begin(), ("-march=" + Triple.getArchName()).str());
   Result.push_back(llvm::join(MArch, "+"));
 

diff  --git a/clang/test/Driver/aarch64-multilib-rcpc3.c b/clang/test/Driver/aarch64-multilib-rcpc3.c
new file mode 100644
index 0000000000000..88b23de5a6510
--- /dev/null
+++ b/clang/test/Driver/aarch64-multilib-rcpc3.c
@@ -0,0 +1,17 @@
+// RUN: %clang --target=aarch64-none-elf -march=armv8.9-a+rcpc3 -print-multi-flags-experimental -c %s 2>&1 | FileCheck %s
+
+// The purpose of this regression test is to make sure that when
+// compile options are converted into multilib selection flags, no
+// empty strings are accidentally included in the
+// -march=armv8.9-a+foo+bar+baz string, leading to two consecutive +
+// signs. With +rcpc3 in the input, this used to generate an empty
+// string for the anonymous architecture extension corresponding to
+// the SubtargetFeature 'rcpc-immo', which is a dependency of rcpc3
+// but has no separate extension name for use on command lines. So we
+// check that the two named rcpc options appear, and that no ++
+// appears before or after.
+
+// CHECK: -march=armv8.9-a
+// CHECK-NOT: ++
+// CHECK-SAME: +rcpc+rcpc3+
+// CHECK-NOT: ++