[Clang][ARM][Sema] Reject bad sizes of __builtin_arm_ldrex #150419
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Depending on the particular version of the AArch32 architecture,
load/store exclusive operations might be available for various subset
of 8, 16, 32, and 64-bit quantities. Sema knew nothing about this and
was accepting all four sizes, leading to a compiler crash at isel time
if you used a size not available on the target architecture.
Now the Sema checking stage emits a more sensible diagnostic, pointing
at the location in the code.
In order to allow Sema to query the set of supported sizes, I've moved
the enum of LDREX_x sizes out of its Arm-specific header into
`TargetInfo.h`.
Also, in order to allow the diagnostic to specify the correct list of
supported sizes, I've filled it with `%select{}`. (The alternative was
to make separate error messages for each different list of sizes.)
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-arm Author: Simon Tatham (statham-arm) ChangesDepending on the particular version of the AArch32 architecture, load/store exclusive operations might be available for various subset of 8, 16, 32, and 64-bit quantities. Sema knew nothing about this and was accepting all four sizes, leading to a compiler crash at isel time if you used a size not available on the target architecture. Now the Sema checking stage emits a more sensible diagnostic, pointing at the location in the code. In order to allow Sema to query the set of supported sizes, I've moved the enum of LDREX_x sizes out of its Arm-specific header into Also, in order to allow the diagnostic to specify the correct list of supported sizes, I've filled it with Full diff: https://github.com/llvm/llvm-project/pull/150419.diff 10 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b2ea65ae111be..d8b1e312c1228 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9329,8 +9329,28 @@ def err_atomic_builtin_pointer_size : Error<
"address argument to atomic builtin must be a pointer to 1,2,4,8 or 16 byte "
"type (%0 invalid)">;
def err_atomic_exclusive_builtin_pointer_size : Error<
- "address argument to load or store exclusive builtin must be a pointer to"
- " 1,2,4 or 8 byte type (%0 invalid)">;
+ "address argument to load or store exclusive builtin must be a pointer to "
+ // Because the range of legal sizes for load/store exclusive varies with the
+ // Arm architecture version, this error message wants to be able to specify
+ // various different subsets of the sizes 1, 2, 4, 8. Rather than make a
+ // separate diagnostic for each subset, I've arranged here that _this_ error
+ // can display any combination of the sizes. For each size there are two
+ // %select parameters: the first chooses whether you need a "," or " or " to
+ // separate the number from a previous one (or neither), and the second
+ // parameter indicates whether to display the number itself.
+ //
+ // (The very first of these parameters isn't really necessary, since you
+ // never want to start with "," or " or " before the first number in the
+ // list, but it keeps it simple to make it look exactly like the other cases,
+ // and also allows a loop constructing this diagnostic to handle every case
+ // exactly the same.)
+ "%select{|,| or }1%select{|1}2"
+ "%select{|,| or }3%select{|2}4"
+ "%select{|,| or }5%select{|4}6"
+ "%select{|,| or }7%select{|8}8"
+ " byte type (%0 invalid)">;
+def err_atomic_exclusive_builtin_pointer_size_none : Error<
+ "load and store exclusive builtins are not available on this architecture">;
def err_atomic_builtin_ext_int_size : Error<
"atomic memory operand must have a power-of-two size">;
def err_atomic_builtin_bit_int_prohibit : Error<
diff --git a/clang/include/clang/Basic/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index 8b66c73474704..7e84c6a9ce0b5 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -1071,6 +1071,17 @@ class TargetInfo : public TransferrableTargetInfo,
/// as Custom Datapath.
uint32_t getARMCDECoprocMask() const { return ARMCDECoprocMask; }
+ /// For ARM targets returns a mask defining which data sizes are suitable for
+ /// __builtin_arm_ldrex and __builtin_arm_strex.
+ enum {
+ ARM_LDREX_B = (1 << 0), /// byte (8-bit)
+ ARM_LDREX_H = (1 << 1), /// half (16-bit)
+ ARM_LDREX_W = (1 << 2), /// word (32-bit)
+ ARM_LDREX_D = (1 << 3), /// double (64-bit)
+ };
+
+ virtual unsigned getARMLDREXMask() const { return 0; }
+
/// Returns whether the passed in string is a valid clobber in an
/// inline asm statement.
///
diff --git a/clang/include/clang/Sema/SemaARM.h b/clang/include/clang/Sema/SemaARM.h
index 788a7abf5f9c1..0f7838bdac70f 100644
--- a/clang/include/clang/Sema/SemaARM.h
+++ b/clang/include/clang/Sema/SemaARM.h
@@ -44,8 +44,8 @@ class SemaARM : public SemaBase {
bool CheckImmediateArg(CallExpr *TheCall, unsigned CheckTy, unsigned ArgIdx,
unsigned EltBitWidth, unsigned VecBitWidth);
- bool CheckARMBuiltinExclusiveCall(unsigned BuiltinID, CallExpr *TheCall,
- unsigned MaxWidth);
+ bool CheckARMBuiltinExclusiveCall(const TargetInfo &TI, unsigned BuiltinID,
+ CallExpr *TheCall);
bool CheckNeonBuiltinFunctionCall(const TargetInfo &TI, unsigned BuiltinID,
CallExpr *TheCall);
bool PerformNeonImmChecks(
diff --git a/clang/lib/Basic/Targets/ARM.cpp b/clang/lib/Basic/Targets/ARM.cpp
index 29de34bbc4fe4..6bec2fae0fbd0 100644
--- a/clang/lib/Basic/Targets/ARM.cpp
+++ b/clang/lib/Basic/Targets/ARM.cpp
@@ -618,21 +618,21 @@ bool ARMTargetInfo::handleTargetFeatures(std::vector<std::string> &Features,
LDREX = 0;
else if (ArchKind == llvm::ARM::ArchKind::ARMV6K ||
ArchKind == llvm::ARM::ArchKind::ARMV6KZ)
- LDREX = LDREX_D | LDREX_W | LDREX_H | LDREX_B;
+ LDREX = ARM_LDREX_D | ARM_LDREX_W | ARM_LDREX_H | ARM_LDREX_B;
else
- LDREX = LDREX_W;
+ LDREX = ARM_LDREX_W;
break;
case 7:
case 8:
if (ArchProfile == llvm::ARM::ProfileKind::M)
- LDREX = LDREX_W | LDREX_H | LDREX_B;
+ LDREX = ARM_LDREX_W | ARM_LDREX_H | ARM_LDREX_B;
else
- LDREX = LDREX_D | LDREX_W | LDREX_H | LDREX_B;
+ LDREX = ARM_LDREX_D | ARM_LDREX_W | ARM_LDREX_H | ARM_LDREX_B;
break;
case 9:
assert(ArchProfile != llvm::ARM::ProfileKind::M &&
"No Armv9-M architectures defined");
- LDREX = LDREX_D | LDREX_W | LDREX_H | LDREX_B;
+ LDREX = ARM_LDREX_D | ARM_LDREX_W | ARM_LDREX_H | ARM_LDREX_B;
}
if (!(FPU & NeonFPU) && FPMath == FP_Neon) {
diff --git a/clang/lib/Basic/Targets/ARM.h b/clang/lib/Basic/Targets/ARM.h
index 1719217ca25b7..43c4718f4735b 100644
--- a/clang/lib/Basic/Targets/ARM.h
+++ b/clang/lib/Basic/Targets/ARM.h
@@ -98,13 +98,6 @@ class LLVM_LIBRARY_VISIBILITY ARMTargetInfo : public TargetInfo {
LLVM_PREFERRED_TYPE(bool)
unsigned HasBTI : 1;
- enum {
- LDREX_B = (1 << 0), /// byte (8-bit)
- LDREX_H = (1 << 1), /// half (16-bit)
- LDREX_W = (1 << 2), /// word (32-bit)
- LDREX_D = (1 << 3), /// double (64-bit)
- };
-
uint32_t LDREX;
// ACLE 6.5.1 Hardware floating point
@@ -225,6 +218,8 @@ class LLVM_LIBRARY_VISIBILITY ARMTargetInfo : public TargetInfo {
bool hasBitIntType() const override { return true; }
+ unsigned getARMLDREXMask() const override { return LDREX; }
+
const char *getBFloat16Mangling() const override { return "u6__bf16"; };
std::pair<unsigned, unsigned> hardwareInterferenceSizes() const override {
diff --git a/clang/lib/Sema/SemaARM.cpp b/clang/lib/Sema/SemaARM.cpp
index bd603a925d15e..d7cef178045fc 100644
--- a/clang/lib/Sema/SemaARM.cpp
+++ b/clang/lib/Sema/SemaARM.cpp
@@ -846,9 +846,9 @@ bool SemaARM::CheckARMCoprocessorImmediate(const TargetInfo &TI,
return false;
}
-bool SemaARM::CheckARMBuiltinExclusiveCall(unsigned BuiltinID,
- CallExpr *TheCall,
- unsigned MaxWidth) {
+bool SemaARM::CheckARMBuiltinExclusiveCall(const TargetInfo &TI,
+ unsigned BuiltinID,
+ CallExpr *TheCall) {
assert((BuiltinID == ARM::BI__builtin_arm_ldrex ||
BuiltinID == ARM::BI__builtin_arm_ldaex ||
BuiltinID == ARM::BI__builtin_arm_strex ||
@@ -923,12 +923,56 @@ bool SemaARM::CheckARMBuiltinExclusiveCall(unsigned BuiltinID,
return true;
}
- // But ARM doesn't have instructions to deal with 128-bit versions.
- if (Context.getTypeSize(ValType) > MaxWidth) {
- assert(MaxWidth == 64 && "Diagnostic unexpectedly inaccurate");
- Diag(DRE->getBeginLoc(), diag::err_atomic_exclusive_builtin_pointer_size)
- << PointerArg->getType() << PointerArg->getSourceRange();
- return true;
+ // Check whether the size of the type can be handled atomically on this
+ // target.
+ if (!TI.getTriple().isAArch64()) {
+ unsigned Mask = TI.getARMLDREXMask();
+ unsigned Bits = Context.getTypeSize(ValType);
+ bool Supported =
+ (llvm::isPowerOf2_64(Bits)) && Bits >= 8 && (Mask & (Bits / 8));
+
+ if (!Supported) {
+ // Emit a diagnostic saying that this size isn't available. If _no_ size
+ // of exclusive access is supported on this target, we emit a diagnostic
+ // with special wording for that case, but otherwise, we emit
+ // err_atomic_exclusive_builtin_pointer_size and loop over `Mask` to
+ // control what subset of sizes it lists as legal.
+ if (Mask) {
+ auto D = Diag(DRE->getBeginLoc(),
+ diag::err_atomic_exclusive_builtin_pointer_size)
+ << PointerArg->getType();
+ bool Started = false;
+ for (unsigned Size = 1; Size <= 8; Size <<= 1) {
+ // For each of the sizes 1,2,4,8, pass two integers into the
+ // diagnostic. The first selects a separator from the previous
+ // number: 0 for no separator at all, 1 for a comma, 2 for " or "
+ // which appears before the final number in a list of more than one.
+ // The second integer just indicates whether we print this size in
+ // the message at all.
+ if (!(Mask & Size)) {
+ // This size isn't one of the supported ones, so emit no separator
+ // text and don't print the size itself.
+ D << 0 << 0;
+ } else {
+ // This size is supported, so print it, and an appropriate
+ // separator.
+ Mask &= ~Size;
+ if (!Started)
+ D << 0; // No separator if this is the first size we've printed
+ else if (Mask)
+ D << 1; // "," if there's still another size to come
+ else
+ D << 2; // " or " if the size we're about to print is the last
+ D << 1; // print the size itself
+ Started = true;
+ }
+ }
+ } else {
+ Diag(DRE->getBeginLoc(),
+ diag::err_atomic_exclusive_builtin_pointer_size_none)
+ << PointerArg->getSourceRange();
+ }
+ }
}
switch (ValType.getObjCLifetime()) {
@@ -972,7 +1016,7 @@ bool SemaARM::CheckARMBuiltinFunctionCall(const TargetInfo &TI,
BuiltinID == ARM::BI__builtin_arm_ldaex ||
BuiltinID == ARM::BI__builtin_arm_strex ||
BuiltinID == ARM::BI__builtin_arm_stlex) {
- return CheckARMBuiltinExclusiveCall(BuiltinID, TheCall, 64);
+ return CheckARMBuiltinExclusiveCall(TI, BuiltinID, TheCall);
}
if (BuiltinID == ARM::BI__builtin_arm_prefetch) {
@@ -1053,7 +1097,7 @@ bool SemaARM::CheckAArch64BuiltinFunctionCall(const TargetInfo &TI,
BuiltinID == AArch64::BI__builtin_arm_ldaex ||
BuiltinID == AArch64::BI__builtin_arm_strex ||
BuiltinID == AArch64::BI__builtin_arm_stlex) {
- return CheckARMBuiltinExclusiveCall(BuiltinID, TheCall, 128);
+ return CheckARMBuiltinExclusiveCall(TI, BuiltinID, TheCall);
}
if (BuiltinID == AArch64::BI__builtin_arm_prefetch) {
diff --git a/clang/test/Sema/builtins-arm-exclusive-124.c b/clang/test/Sema/builtins-arm-exclusive-124.c
new file mode 100644
index 0000000000000..013ae3f41ee7f
--- /dev/null
+++ b/clang/test/Sema/builtins-arm-exclusive-124.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple armv7m -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple armv8m.main -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple armv8.1m.main -fsyntax-only -verify %s
+
+// All these architecture versions provide 1-, 2- or 4-byte exclusive accesses,
+// but don't have the LDREXD instruction which takes two operand registers and
+// performs an 8-byte exclusive access. So the calls with a pointer to long
+// long are rejected.
+
+int test_ldrex(char *addr) {
+ int sum = 0;
+ sum += __builtin_arm_ldrex(addr);
+ sum += __builtin_arm_ldrex((short *)addr);
+ sum += __builtin_arm_ldrex((int *)addr);
+ sum += __builtin_arm_ldrex((long long *)addr); // expected-error {{address argument to load or store exclusive builtin must be a pointer to 1,2 or 4 byte type}}
+ return sum;
+}
+
+int test_strex(char *addr) {
+ int res = 0;
+ res |= __builtin_arm_strex(4, addr);
+ res |= __builtin_arm_strex(42, (short *)addr);
+ res |= __builtin_arm_strex(42, (int *)addr);
+ res |= __builtin_arm_strex(42, (long long *)addr); // expected-error {{address argument to load or store exclusive builtin must be a pointer to 1,2 or 4 byte type}}
+ return res;
+}
diff --git a/clang/test/Sema/builtins-arm-exclusive-4.c b/clang/test/Sema/builtins-arm-exclusive-4.c
new file mode 100644
index 0000000000000..68f01f5416616
--- /dev/null
+++ b/clang/test/Sema/builtins-arm-exclusive-4.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple armv6 -fsyntax-only -verify %s
+
+// Armv6 (apart from Armv6-M) provides 4-byte exclusive accesses, but not any
+// other size. So only the calls with a pointer to a 32-bit type are accepted.
+
+int test_ldrex(char *addr) {
+ int sum = 0;
+ sum += __builtin_arm_ldrex(addr); // expected-error {{address argument to load or store exclusive builtin must be a pointer to 4 byte type}}
+ sum += __builtin_arm_ldrex((short *)addr); // expected-error {{address argument to load or store exclusive builtin must be a pointer to 4 byte type}}
+ sum += __builtin_arm_ldrex((int *)addr);
+ sum += __builtin_arm_ldrex((long long *)addr); // expected-error {{address argument to load or store exclusive builtin must be a pointer to 4 byte type}}
+ return sum;
+}
+
+int test_strex(char *addr) {
+ int res = 0;
+ res |= __builtin_arm_strex(4, addr); // expected-error {{address argument to load or store exclusive builtin must be a pointer to 4 byte type}}
+ res |= __builtin_arm_strex(42, (short *)addr); // expected-error {{address argument to load or store exclusive builtin must be a pointer to 4 byte type}}
+ res |= __builtin_arm_strex(42, (int *)addr);
+ res |= __builtin_arm_strex(42, (long long *)addr); // expected-error {{address argument to load or store exclusive builtin must be a pointer to 4 byte type}}
+ return res;
+}
diff --git a/clang/test/Sema/builtins-arm-exclusive-none.c b/clang/test/Sema/builtins-arm-exclusive-none.c
new file mode 100644
index 0000000000000..76d327f0111c3
--- /dev/null
+++ b/clang/test/Sema/builtins-arm-exclusive-none.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple armv6m -fsyntax-only -verify %s
+
+// Armv6-M does not support exclusive loads/stores at all, so all uses of
+// __builtin_arm_ldrex and __builtin_arm_strex is forbidden.
+
+int test_ldrex(char *addr) {
+ int sum = 0;
+ sum += __builtin_arm_ldrex(addr); // expected-error {{load and store exclusive builtins are not available on this architecture}}
+ sum += __builtin_arm_ldrex((short *)addr); // expected-error {{load and store exclusive builtins are not available on this architecture}}
+ sum += __builtin_arm_ldrex((int *)addr); // expected-error {{load and store exclusive builtins are not available on this architecture}}
+ sum += __builtin_arm_ldrex((long long *)addr); // expected-error {{load and store exclusive builtins are not available on this architecture}}
+ return sum;
+}
+
+int test_strex(char *addr) {
+ int res = 0;
+ res |= __builtin_arm_strex(4, addr); // expected-error {{load and store exclusive builtins are not available on this architecture}}
+ res |= __builtin_arm_strex(42, (short *)addr); // expected-error {{load and store exclusive builtins are not available on this architecture}}
+ res |= __builtin_arm_strex(42, (int *)addr); // expected-error {{load and store exclusive builtins are not available on this architecture}}
+ res |= __builtin_arm_strex(42, (long long *)addr); // expected-error {{load and store exclusive builtins are not available on this architecture}}
+ return res;
+}
diff --git a/clang/test/Sema/builtins-arm-exclusive.c b/clang/test/Sema/builtins-arm-exclusive.c
index 68457d266b991..49aea1506f25c 100644
--- a/clang/test/Sema/builtins-arm-exclusive.c
+++ b/clang/test/Sema/builtins-arm-exclusive.c
@@ -1,5 +1,13 @@
// RUN: %clang_cc1 -triple armv7 -fsyntax-only -verify %s
+// General tests of __builtin_arm_ldrex and __builtin_arm_strex error checking.
+//
+// This test is compiled for Armv7-A, which provides exclusive load/store
+// instructions for 1-, 2-, 4- and 8-byte quantities. Other Arm architecture
+// versions provide subsets of those, requiring different error reporting.
+// Those are tested in builtins-arm-exclusive-124.c, builtins-arm-exclusive-4.c
+// and builtins-arm-exclusive-none.c.
+
struct Simple {
char a, b;
};
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depending on the particular version of the AArch32 architecture, load/store exclusive operations might be available for various subset of 8, 16, 32, and 64-bit quantities. Sema knew nothing about this and was accepting all four sizes, leading to a compiler crash at isel time if you used a size not available on the target architecture.
Now the Sema checking stage emits a more sensible diagnostic, pointing at the location in the code.
In order to allow Sema to query the set of supported sizes, I've moved the enum of LDREX_x sizes out of its Arm-specific header into
TargetInfo.h.Also, in order to allow the diagnostic to specify the correct list of supported sizes, I've filled it with
%select{}. (The alternative was to make separate error messages for each different list of sizes.)