[Clang][Driver] Skip empty strings in getAArch64MultilibFlags #97827
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Simon Tatham (statham-arm) ChangesIn a multilib setting, if you compile with a command line such as The To fix this, I've excluded extensions from consideration in I've also made the analogous change in Full diff: https://github.com/llvm/llvm-project/pull/97827.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 977e08390800d..9ac428caab3b9 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..b839079e0442d
--- /dev/null
+++ b/clang/test/Driver/aarch64-multilib-rcpc3.c
@@ -0,0 +1,4 @@
+// RUN: %clang --target=aarch64-none-elf -march=armv8.9-a+rcpc3 -print-multi-flags-experimental -c %s 2>&1 | FileCheck %s
+
+// CHECK: -march=armv8.9-a
+// CHECK-SAME: +rcpc+rcpc3+
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
statham-arm
force-pushed
the
multilib-rcpc3-fix
branch
from
8b39cbb to
81d77bf
Compare
| @@ -0,0 +1,4 @@ | |||
| // RUN: %clang --target=aarch64-none-elf -march=armv8.9-a+rcpc3 -print-multi-flags-experimental -c %s 2>&1 | FileCheck %s | |||
|
|
|||
There was a problem hiding this comment.
I think it could be worth a comment here explaining what the test is checking for as it would likely need git blame to work it out otherwise. For example:
// rcpc3 implies rcpc-immo which does not have a user-visible name, check that an empty string is not produced.
| // RUN: %clang --target=aarch64-none-elf -march=armv8.9-a+rcpc3 -print-multi-flags-experimental -c %s 2>&1 | FileCheck %s | ||
|
|
||
| // CHECK: -march=armv8.9-a | ||
| // CHECK-SAME: +rcpc+rcpc3+ |
There was a problem hiding this comment.
I'm wondering if a CHECK-NOT ++ could be used as well. This test might unexpectedly pass if the ordering of the features were changed.
There was a problem hiding this comment.
Done. I wasn't sure whether it would need to come before or after the rcpc+rcpc3 check. The answer turns out to be both.
…7827) 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.
|
Hi, is it expected that this causes flags like From my understanding, this is not a desired behavior ? This is causing failures in google compilation so if my understanding is right please provide a fix forward or revert.. Thanks |
|
@asmok-g , I'm confused. This commit doesn't have anything to do with the processing of Are you sure you've commented on the right PR? If you have, can you provide a full example command line? What was the behaviour before, and what is it afterwards? Why do you say that this commit has caused the change? |
|
I can't find a The closest I can find is https://clang.llvm.org/docs/DiagnosticsReference.html#wc-20-compat which has a text string but this doesn't seem to have an individual warning. Did you mean to use |
|
@statham-arm you're right. I need to verify this is the real culprit as I didn't fully build the toolchain at the previous revision. I used a fast build instead, so I need to verify.. @smithp35 it is actually |
In a multilib setting, if you compile with a command line such as
clang --target=aarch64-none-elf -march=armv8.9-a+rcpc3,getAArch64MultilibFlagsreturns 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 inAArch64::Extensionsfor the SubtargetFeaturercpc-immo, which is a dependency of thercpc3SubtargetFeature, but doesn't have an extension name for the purposes of the-march=foo+baroption. So itsUserVisibleNamefield is the empty string.To fix this, I've excluded extensions from consideration in
getAArch64MultilibFlagsif they have an emptyUserVisibleName. 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 inARMTargetParser.defrather 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.