[clang-tools-extra] 473eff1 - [clang-tidy] Fix crash and handle AttributedType in 'bugprone-easily-swappable-parameters'
via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 22 01:40:15 PDT 2021
Author: Whisperity
Date: 2021-07-22T10:20:17+02:00
New Revision: 473eff1c3057185758bf35f0c052a873b1bdb6a9
URL: https://github.com/llvm/llvm-project/commit/473eff1c3057185758bf35f0c052a873b1bdb6a9
DIFF: https://github.com/llvm/llvm-project/commit/473eff1c3057185758bf35f0c052a873b1bdb6a9.diff
LOG: [clang-tidy] Fix crash and handle AttributedType in 'bugprone-easily-swappable-parameters'
@vabridgers identified a way to crash the check by running on code that
involve `AttributedType`s. This patch fixes the check to first and
foremost not crash, but also improves the logic handling qualifiers.
If the types contain any additional (not just CVR) qualifiers that are
not the same, they will not be deemed mixable. The logic for CVR-Mixing
and the `QualifiersMix` check option remain unchanged.
Reviewed By: aaron.ballman, vabridgers
Differential Revision: http://reviews.llvm.org/D106361
Added:
Modified:
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
index 8e972298adcec..3cb4734e553b0 100644
--- a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
@@ -470,17 +470,18 @@ struct MixData {
}
/// Add the specified qualifiers to the common type in the Mix.
- MixData qualify(Qualifiers Quals) const {
- SplitQualType Split = CommonType.split();
- Split.Quals.addQualifiers(Quals);
- QualType CommonType{Split.Ty, Split.Quals.getAsOpaqueValue()};
+ MixData qualify(Qualifiers Quals, const ASTContext &Ctx) const {
+ if (CommonType.isNull())
+ return *this;
+
+ QualType NewCommonType = Ctx.getQualifiedType(CommonType, Quals);
if (CreatedFromOneWayConversion) {
MixData M{Flags, Conversion};
- M.CommonType = CommonType;
+ M.CommonType = NewCommonType;
return M;
}
- return {Flags, CommonType, Conversion, ConversionRTL};
+ return {Flags, NewCommonType, Conversion, ConversionRTL};
}
};
@@ -566,7 +567,41 @@ approximateImplicitConversion(const TheCheck &Check, QualType LType,
ImplicitConversionModellingMode ImplicitMode);
static inline bool isUselessSugar(const Type *T) {
- return isa<DecayedType, ElaboratedType, ParenType>(T);
+ return isa<AttributedType, DecayedType, ElaboratedType, ParenType>(T);
+}
+
+/// Returns if the two types are qualified in a way that ever after equating or
+/// removing local CVR qualification, even if the unqualified types would mix,
+/// the qualified ones don't, because there are some other local qualifiers
+/// that aren't equal.
+static bool hasNonCVRMixabilityBreakingQualifiers(const ASTContext &Ctx,
+ QualType LType,
+ QualType RType) {
+ LLVM_DEBUG(
+ llvm::dbgs() << ">>> hasNonCVRMixabilityBreakingQualifiers for LType:\n";
+ LType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << "\nand RType:\n";
+ RType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << '\n';);
+ Qualifiers LQual = LType.getLocalQualifiers(),
+ RQual = RType.getLocalQualifiers();
+
+ // Strip potential CVR. That is handled by the check option QualifiersMix.
+ LQual.removeCVRQualifiers();
+ RQual.removeCVRQualifiers();
+
+ Qualifiers CommonQuals = Qualifiers::removeCommonQualifiers(LQual, RQual);
+ (void)CommonQuals;
+
+ LLVM_DEBUG(llvm::dbgs() << "--- hasNonCVRMixabilityBreakingQualifiers. "
+ "Removed common qualifiers: ";
+ CommonQuals.print(llvm::dbgs(), Ctx.getPrintingPolicy());
+ llvm::dbgs() << "\n\tremaining on LType: ";
+ LQual.print(llvm::dbgs(), Ctx.getPrintingPolicy());
+ llvm::dbgs() << "\n\tremaining on RType: ";
+ RQual.print(llvm::dbgs(), Ctx.getPrintingPolicy());
+ llvm::dbgs() << '\n';);
+
+ // If there are any remaining qualifiers, consider the two types distinct.
+ return LQual.hasQualifiers() || RQual.hasQualifiers();
}
/// Approximate the way how LType and RType might refer to "essentially the
@@ -585,12 +620,6 @@ calculateMixability(const TheCheck &Check, QualType LType, QualType RType,
LLVM_DEBUG(llvm::dbgs() << ">>> calculateMixability for LType:\n";
LType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << "\nand RType:\n";
RType.dump(llvm::dbgs(), Ctx); llvm::dbgs() << '\n';);
-
- // Certain constructs match on the last catch-all getCanonicalType() equality,
- // which is perhaps something not what we want. If this variable is true,
- // the canonical type equality will be ignored.
- bool RecursiveReturnDiscardingCanonicalType = false;
-
if (LType == RType) {
LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. Trivial equality.\n");
return {MixFlags::Trivial, LType};
@@ -628,7 +657,6 @@ calculateMixability(const TheCheck &Check, QualType LType, QualType RType,
MixFlags::ReferenceBind;
}
- // Dissolve typedefs after the qualifiers outside the typedef are dealt with.
if (LType->getAs<TypedefType>()) {
LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is typedef.\n");
return calculateMixability(Check, LType.getSingleStepDesugaredType(Ctx),
@@ -645,35 +673,72 @@ calculateMixability(const TheCheck &Check, QualType LType, QualType RType,
// A parameter of type 'cvr1 T' and another of potentially
diff erently
// qualified 'cvr2 T' may bind with the same power, if the user so requested.
+ //
+ // Whether to do this check for the inner unqualified types.
+ bool CompareUnqualifiedTypes = false;
+ // Should the result have a common inner type qualified for diagnosis?
+ bool RequalifyUnqualifiedMixabilityResult = false;
if (LType.getLocalCVRQualifiers() != RType.getLocalCVRQualifiers()) {
- LLVM_DEBUG(if (LType.getLocalCVRQualifiers()) llvm::dbgs()
- << "--- calculateMixability. LHS is CVR.\n");
- LLVM_DEBUG(if (RType.getLocalCVRQualifiers()) llvm::dbgs()
- << "--- calculateMixability. RHS is CVR.\n");
+ LLVM_DEBUG(if (LType.getLocalCVRQualifiers()) {
+ llvm::dbgs() << "--- calculateMixability. LHS has CVR-Qualifiers: ";
+ Qualifiers::fromCVRMask(LType.getLocalCVRQualifiers())
+ .print(llvm::dbgs(), Ctx.getPrintingPolicy());
+ llvm::dbgs() << '\n';
+ });
+ LLVM_DEBUG(if (RType.getLocalCVRQualifiers()) {
+ llvm::dbgs() << "--- calculateMixability. RHS has CVR-Qualifiers: ";
+ Qualifiers::fromCVRMask(RType.getLocalCVRQualifiers())
+ .print(llvm::dbgs(), Ctx.getPrintingPolicy());
+ llvm::dbgs() << '\n';
+ });
if (!Check.QualifiersMix) {
LLVM_DEBUG(llvm::dbgs()
- << "<<< calculateMixability. QualifiersMix turned off.\n");
+ << "<<< calculateMixability. QualifiersMix turned off - not "
+ "mixable.\n");
return {MixFlags::None};
}
- return calculateMixability(Check, LType.getLocalUnqualifiedType(),
- RType.getLocalUnqualifiedType(), Ctx,
- ImplicitMode) |
- MixFlags::Qualifiers;
+ CompareUnqualifiedTypes = true;
}
if (LType.getLocalCVRQualifiers() == RType.getLocalCVRQualifiers() &&
LType.getLocalCVRQualifiers() != 0) {
- LLVM_DEBUG(llvm::dbgs()
- << "--- calculateMixability. LHS and RHS same CVR.\n");
+ LLVM_DEBUG(if (LType.getLocalCVRQualifiers()) {
+ llvm::dbgs()
+ << "--- calculateMixability. LHS and RHS have same CVR-Qualifiers: ";
+ Qualifiers::fromCVRMask(LType.getLocalCVRQualifiers())
+ .print(llvm::dbgs(), Ctx.getPrintingPolicy());
+ llvm::dbgs() << '\n';
+ });
+
+ CompareUnqualifiedTypes = true;
+ RequalifyUnqualifiedMixabilityResult = true;
+ }
+
+ if (CompareUnqualifiedTypes) {
+ if (hasNonCVRMixabilityBreakingQualifiers(Ctx, LType, RType)) {
+ LLVM_DEBUG(llvm::dbgs() << "<<< calculateMixability. Additional "
+ "non-equal incompatible qualifiers.\n");
+ return {MixFlags::None};
+ }
+
+ MixData UnqualifiedMixability =
+ calculateMixability(Check, LType.getLocalUnqualifiedType(),
+ RType.getLocalUnqualifiedType(), Ctx, ImplicitMode);
+
+ if (!RequalifyUnqualifiedMixabilityResult)
+ return UnqualifiedMixability | MixFlags::Qualifiers;
+
// Apply the same qualifier back into the found common type if we found
// a common type between the unqualified versions.
- return calculateMixability(Check, LType.getLocalUnqualifiedType(),
- RType.getLocalUnqualifiedType(), Ctx,
- ImplicitMode)
- .qualify(LType.getLocalQualifiers());
+ return UnqualifiedMixability.qualify(LType.getLocalQualifiers(), Ctx);
}
+ // Certain constructs match on the last catch-all getCanonicalType() equality,
+ // which is perhaps something not what we want. If this variable is true,
+ // the canonical type equality will be ignored.
+ bool RecursiveReturnDiscardingCanonicalType = false;
+
if (LType->isPointerType() && RType->isPointerType()) {
// If both types are pointers, and pointed to the exact same type,
// LType == RType took care of that. Try to see if the pointee type has
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
index e2e836b67cc33..58138c720a399 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
@@ -346,3 +346,31 @@ void memberTypedefDependentReference3(
void functionPrototypeLosesNoexcept(void (*NonThrowing)() noexcept, void (*Throwing)()) {}
// NO-WARN: This call cannot be swapped, even if "getCanonicalType()" believes otherwise.
+
+void attributedParam1(const __attribute__((address_space(256))) int *One,
+ const __attribute__((address_space(256))) int *Two) {}
+// CHECK-MESSAGES: :[[@LINE-2]]:23: warning: 2 adjacent parameters of 'attributedParam1' of similar type ('const __attribute__((address_space(256))) int *') are
+// CHECK-MESSAGES: :[[@LINE-3]]:70: note: the first parameter in the range is 'One'
+// CHECK-MESSAGES: :[[@LINE-3]]:70: note: the last parameter in the range is 'Two'
+
+void attributedParam1Typedef(const __attribute__((address_space(256))) int *One,
+ const __attribute__((address_space(256))) MyInt1 *Two) {}
+// CHECK-MESSAGES: :[[@LINE-2]]:30: warning: 2 adjacent parameters of 'attributedParam1Typedef' of similar type are
+// CHECK-MESSAGES: :[[@LINE-3]]:77: note: the first parameter in the range is 'One'
+// CHECK-MESSAGES: :[[@LINE-3]]:80: note: the last parameter in the range is 'Two'
+// CHECK-MESSAGES: :[[@LINE-5]]:30: note: after resolving type aliases, the common type of 'const __attribute__((address_space(256))) int *' and 'const __attribute__((address_space(256))) MyInt1 *' is 'const __attribute__((address_space(256))) int'
+// FIXME: The last diagnostic line is a bit bad: the common type should be a
+// pointer type -- it is not clear right now, how it would be possible to
+// properly wire a logic in that fixes it.
+
+void attributedParam2(__attribute__((address_space(256))) int *One,
+ const __attribute__((address_space(256))) MyInt1 *Two) {}
+// NO-WARN: One is CVR-qualified, the other is not.
+
+void attributedParam3(const int *One,
+ const __attribute__((address_space(256))) MyInt1 *Two) {}
+// NO-WARN: One is attributed, the other is not.
+
+void attributedParam4(const __attribute__((address_space(512))) int *One,
+ const __attribute__((address_space(256))) MyInt1 *Two) {}
+// NO-WARN: Different value of the attribute.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
index 9ba81a484163e..d51f44972dcb8 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
@@ -113,3 +113,14 @@ void copy(const T *Dest, T *Source) {}
// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'Dest'
// CHECK-MESSAGES: :[[@LINE-3]]:29: note: the last parameter in the range is 'Source'
// CHECK-MESSAGES: :[[@LINE-4]]:26: note: 'const T *' and 'T *' parameters accept and bind the same kind of values
+
+void attributedParam2(__attribute__((address_space(256))) int *One,
+ const __attribute__((address_space(256))) MyInt1 *Two) {}
+// CHECK-MESSAGES: :[[@LINE-2]]:23: warning: 2 adjacent parameters of 'attributedParam2' of similar type are
+// CHECK-MESSAGES: :[[@LINE-3]]:64: note: the first parameter in the range is 'One'
+// CHECK-MESSAGES: :[[@LINE-3]]:73: note: the last parameter in the range is 'Two'
+// CHECK-MESSAGES: :[[@LINE-5]]:23: note: after resolving type aliases, the common type of '__attribute__((address_space(256))) int *' and 'const __attribute__((address_space(256))) MyInt1 *' is 'int'
+// CHECK-MESSAGES: :[[@LINE-5]]:23: note: '__attribute__((address_space(256))) int *' and 'const __attribute__((address_space(256))) MyInt1 *' parameters accept and bind the same kind of values
+// FIXME: The last diagnostic line is a bit bad: the common type should be a
+// pointer type -- it is not clear right now, how it would be possible to
+// properly wire a logic in that fixes it.
More information about the cfe-commits
mailing list