steakhal created this revision.
steakhal added reviewers: aaron.ballman, whisperity, alexfh, hokein, JonasToth,
courbet, ymandel.
Herald added subscribers: carlosgalvezp, martong, rnkovacs, kbarton,
kristof.beyls, xazax.hun, nemanjai.
steakhal requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Consider this example:
struct SmallBitfield { unsigned int id : 4; } x;
x.id << 1;
The corresponding AST looks like this:
BinaryOperator 'int' '<<'
|-ImplicitCastExpr 'int' <IntegralCast>
| `-ImplicitCastExpr 'unsigned int' <LValueToRValue>
| `-MemberExpr 'unsigned int' lvalue bitfield .id
| `-DeclRefExpr 'SmallBitfield' lvalue ParmVar 'x' 'SmallBitfield'
`-IntegerLiteral 'int' 1
There are two implicit casts:
1. LValueToRValue, loading from the bitfield
2. IntegralCast, casting the 'unsigned int' to 'int' to process the bitshift
operation
This patch aims to detect this case and ignore it.
The patch might be too restrictive in terms of matching for only this
exact pattern, but I did not find any other similar occurrences of this
kind.
https://reviews.llvm.org/D114105
Files:
clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp
@@ -0,0 +1,107 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t \
+// RUN: -std=c++17 -- -target x86_64-unknown-linux
+
+#define CHAR_BITS 8
+static_assert(sizeof(unsigned int) == 32 / CHAR_BITS);
+
+template <typename T, typename U>
+struct is_same {
+ static constexpr bool value = false;
+};
+template <typename T>
+struct is_same<T, T> {
+ static constexpr bool value = true;
+};
+
+template <typename T, typename U>
+static constexpr bool is_same_v = is_same<T, U>::value;
+
+struct NoBitfield {
+ unsigned int id;
+};
+struct SmallBitfield {
+ unsigned int id : 4;
+};
+struct BigBitfield {
+ unsigned int id : 32;
+};
+
+void test_binary_and(SmallBitfield x) {
+ // CHECK-MESSAGES: [[@LINE+1]]:36: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ static_assert(is_same_v<decltype(x.id & 1), int>);
+ static_assert(is_same_v<decltype(x.id & 1u), unsigned>); // no-warning
+
+ // CHECK-MESSAGES: [[@LINE+1]]:3: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ x.id & 1;
+ x.id & 1u; // no-warning
+
+ // CHECK-MESSAGES: [[@LINE+1]]:7: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ 1 & x.id;
+ 1u & x.id; // no-warning
+}
+
+void test_binary_or(SmallBitfield x) {
+ // CHECK-MESSAGES: [[@LINE+1]]:36: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ static_assert(is_same_v<decltype(x.id | 1), int>);
+ static_assert(is_same_v<decltype(x.id | 1u), unsigned>); // no-warning
+
+ // CHECK-MESSAGES: [[@LINE+1]]:3: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ x.id | 1;
+ x.id | 1u; // no-warning
+
+ // CHECK-MESSAGES: [[@LINE+1]]:7: warning: narrowing conversion from 'unsigned int' to signed type 'int' is implementation-defined [cppcoreguidelines-narrowing-conversions]
+ 1 | x.id;
+ 1u | x.id; // no-warning
+}
+
+void test(NoBitfield x) {
+ // no-warning in this function
+ static_assert(is_same_v<decltype(x.id << 1), unsigned>);
+ static_assert(is_same_v<decltype(x.id << 1u), unsigned>);
+
+ x.id << 1;
+ x.id << 1u;
+ x.id >> 1;
+ x.id >> 1u;
+
+ 1 << x.id;
+ 1u << x.id;
+ 1 >> x.id;
+ 1u >> x.id;
+}
+
+void test(SmallBitfield x) {
+ // no-warning in this function: bitfield reads should be ignored within shift operations
+
+ // 'int' is large enough to hold the value of 'x.id', since that could at
+ // most populate 4 bits and an 'int' has 32.
+ static_assert(is_same_v<decltype(x.id << 1), int>);
+ static_assert(is_same_v<decltype(x.id << 1u), int>);
+
+ x.id << 1;
+ x.id << 1u;
+ x.id >> 1;
+ x.id >> 1u;
+
+ 1 << x.id;
+ 1u << x.id;
+ 1 >> x.id;
+ 1u >> x.id;
+}
+
+void test(BigBitfield x) {
+ // no-warning in this function: there is no int -> unsigned conversion here
+
+ static_assert(is_same_v<decltype(x.id << 1), unsigned>);
+ static_assert(is_same_v<decltype(x.id << 1u), unsigned>);
+
+ x.id << 1;
+ x.id << 1u;
+ x.id >> 1;
+ x.id >> 1u;
+
+ 1 << x.id;
+ 1u << x.id;
+ 1 >> x.id;
+ 1u >> x.id;
+}
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
@@ -83,6 +83,28 @@
binaryOperator(hasOperands(IsConversionFromIgnoredType,
hasType(isInteger()))));
+ // Reading a small unsigned bitfield member will be wrapped by an implicit
+ // cast to 'int' triggering this checker. But this is known to be safe by the
+ // compiler since it chose 'int' instead of 'unsigned int' as the type of the
+ // whole expression, so let's ignore this case.
+ //
+ // struct SmallBitfield { unsigned int id : 4; };
+ // x.id << 1;
+ //
+ // BinaryOperator 'int' '<<'
+ // |-ImplicitCastExpr 'int' <IntegralCast>
+ // | `-ImplicitCastExpr 'unsigned int' <LValueToRValue>
+ // | `-MemberExpr 'unsigned int' lvalue bitfield .id
+ // | `-DeclRefExpr 'SmallBitfield' lvalue ParmVar 'x' 'SmallBitfield'
+ // `-IntegerLiteral 'int' 1
+ const auto ShiftingWidenedBitfieldValue = castExpr(
+ hasCastKind(CK_IntegralCast), hasType(asString("int")),
+ has(castExpr(
+ castExpr(hasCastKind(CK_LValueToRValue),
+ has(memberExpr(hasDeclaration(fieldDecl(isBitField()))))))),
+ hasParent(
+ binaryOperator(anyOf(hasOperatorName("<<"), hasOperatorName(">>")))));
+
// Casts:
// i = 0.5;
// void f(int); f(0.5);
@@ -100,7 +122,8 @@
IgnoreConversionFromTypes.empty()
? castExpr()
: castExpr(unless(hasSourceExpression(
- IsIgnoredTypeTwoLevelsDeep))))
+ IsIgnoredTypeTwoLevelsDeep))),
+ unless(ShiftingWidenedBitfieldValue))
.bind("cast")),
this);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
