[clang] [compiler-rt] Fix "[clang][UBSan] Add implicit conversion check for bitfields" (PR #87761)
@@ -0,0 +1,94 @@
+// RUN: %clang -x c++ -fsanitize=implicit-bitfield-conversion -target
x86_64-linux -S -emit-llvm -o - %s | FileCheck %s
--check-prefixes=CHECK,CHECK-BITFIELD-CONVERSION
+// RUN: %clang -x c++ -fsanitize=implicit-integer-conversion -target
x86_64-linux -S -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK
+// RUN: %clang -x c++ -fsanitize=implicit-conversion -target x86_64-linux -S
-emit-llvm -o - %s | FileCheck %s
--check-prefixes=CHECK,CHECK-BITFIELD-CONVERSION
+
+struct S {
+ int a:3;
+ char b:2;
+};
+
+class C : public S {
+ public:
+short c:3;
+};
+
+S s;
+C c;
+
+// CHECK-LABEL: define{{.*}} void @{{.*foo1.*}}
+void foo1(int x) {
+ s.a = x;
+ // CHECK: store i8 %{{.*}}
+ // CHECK-BITFIELD-CONVERSION: [[BFRESULTSHL:%.*]] = shl i8 {{.*}}, 5
+ // CHECK-BITFIELD-CONVERSION-NEXT: [[BFRESULTASHR:%.*]] = ashr i8
[[BFRESULTSHL]], 5
+ // CHECK-BITFIELD-CONVERSION-NEXT: [[BFRESULTCAST:%.*]] = sext i8
[[BFRESULTASHR]] to i32
+ // CHECK-BITFIELD-CONVERSION: call void @__ubsan_handle_implicit_conversion
+ // CHECK-BITFIELD-CONVERSION-NEXT: br label %[[CONT:.*]], !nosanitize !6
Zonotora wrote:
No, feel free to update
https://github.com/llvm/llvm-project/pull/87761
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] Fix "[clang][UBSan] Add implicit conversion check for bitfields" (PR #87761)
Zonotora wrote: @vitalybuka Well, this is awkward, fails due to: https://lab.llvm.org/buildbot/#/builders/127/builds/64260 C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\test\ubsan\TestCases\ImplicitConversion\bitfield-conversion.c:36:3: error: unknown type name '_Bool' Some other testcases failing as well. Seems to be flaky, no? https://github.com/llvm/llvm-project/pull/87761 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] Fix "[clang][UBSan] Add implicit conversion check for bitfields" (PR #87761)
Zonotora wrote: @vitalybuka all green? https://github.com/llvm/llvm-project/pull/87761 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] Fix "[clang][UBSan] Add implicit conversion check for bitfields" (PR #87761)
Zonotora wrote: @vitalybuka yes, seems like all testcases failed due to not explicitly setting BitfieldBits to 0 except one, LeakSanitizer-AddressSanitizer-i386 :: TestCases/leak_check_at_exit.cpp not really sure what the problem is. Also added another testcase `ubsan/TestCases/ImplicitConversion/bitfield-conversion.c` to make sure it works properly. https://github.com/llvm/llvm-project/pull/87761 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] Fix "[clang][UBSan] Add implicit conversion check for bitfields" (PR #87761)
https://github.com/Zonotora edited https://github.com/llvm/llvm-project/pull/87761 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] Fix "[clang][UBSan] Add implicit conversion check for bitfields" (PR #87761)
https://github.com/Zonotora updated https://github.com/llvm/llvm-project/pull/87761 >From a2a1ba32654f6b4a6b3d85e7a8733c9f8bd1b5f7 Mon Sep 17 00:00:00 2001 From: Zonotora Date: Thu, 4 Apr 2024 20:27:14 +0200 Subject: [PATCH] Fix "[clang][UBSan] Add implicit conversion check for bitfields" --- clang/docs/ReleaseNotes.rst | 7 + clang/docs/UndefinedBehaviorSanitizer.rst | 19 +- clang/include/clang/Basic/Sanitizers.def | 20 +- clang/lib/CodeGen/CGExpr.cpp | 37 +- clang/lib/CodeGen/CGExprScalar.cpp| 265 +++- clang/lib/CodeGen/CodeGenFunction.h | 15 + .../test/CodeGen/ubsan-bitfield-conversion.c | 61 ++ .../CodeGenCXX/ubsan-bitfield-conversion.cpp | 94 +++ clang/test/Driver/fsanitize.c | 28 +- compiler-rt/lib/ubsan/ubsan_handlers.cpp | 27 +- compiler-rt/lib/ubsan/ubsan_handlers.h| 1 + .../ImplicitConversion/bitfield-conversion.c | 639 ++ 12 files changed, 1138 insertions(+), 75 deletions(-) create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c create mode 100644 clang/test/CodeGenCXX/ubsan-bitfield-conversion.cpp create mode 100644 compiler-rt/test/ubsan/TestCases/ImplicitConversion/bitfield-conversion.c diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 28e8ddb3c41c3e9..abf15c8dc49d9e7 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -198,6 +198,10 @@ Non-comprehensive list of changes in this release New Compiler Flags -- +- ``-fsanitize=implicit-bitfield-conversion`` checks implicit truncation and + sign change. +- ``-fsanitize=implicit-integer-conversion`` a group that replaces the previous + group ``-fsanitize=implicit-conversion``. - ``-Wmissing-designated-field-initializers``, grouped under ``-Wmissing-field-initializers``. This diagnostic can be disabled to make ``-Wmissing-field-initializers`` behave @@ -211,6 +215,9 @@ Modified Compiler Flags - Added a new diagnostic flag ``-Wreturn-mismatch`` which is grouped under ``-Wreturn-type``, and moved some of the diagnostics previously controlled by ``-Wreturn-type`` under this new flag. Fixes #GH72116. +- ``-fsanitize=implicit-conversion`` is now a group for both + ``-fsanitize=implicit-integer-conversion`` and + ``-fsanitize=implicit-bitfield-conversion``. - Added ``-Wcast-function-type-mismatch`` under the ``-Wcast-function-type`` warning group. Moved the diagnostic previously controlled by diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst b/clang/docs/UndefinedBehaviorSanitizer.rst index 8f58c92bd2a1634..531d56e313826c7 100644 --- a/clang/docs/UndefinedBehaviorSanitizer.rst +++ b/clang/docs/UndefinedBehaviorSanitizer.rst @@ -148,6 +148,11 @@ Available checks are: Issues caught by this sanitizer are not undefined behavior, but are often unintentional. - ``-fsanitize=integer-divide-by-zero``: Integer division by zero. + - ``-fsanitize=implicit-bitfield-conversion``: Implicit conversion from + integer of larger bit width to smaller bitfield, if that results in data + loss. This includes unsigned/signed truncations and sign changes, similarly + to how the ``-fsanitize=implicit-integer-conversion`` group works, but + explicitly for bitfields. - ``-fsanitize=nonnull-attribute``: Passing null pointer as a function parameter which is declared to never be null. - ``-fsanitize=null``: Use of a null pointer or creation of a null @@ -193,8 +198,8 @@ Available checks are: signed division overflow (``INT_MIN/-1``). Note that checks are still added even when ``-fwrapv`` is enabled. This sanitizer does not check for lossy implicit conversions performed before the computation (see - ``-fsanitize=implicit-conversion``). Both of these two issues are handled - by ``-fsanitize=implicit-conversion`` group of checks. + ``-fsanitize=implicit-integer-conversion``). Both of these two issues are handled + by ``-fsanitize=implicit-integer-conversion`` group of checks. - ``-fsanitize=unreachable``: If control flow reaches an unreachable program point. - ``-fsanitize=unsigned-integer-overflow``: Unsigned integer overflow, where @@ -202,7 +207,7 @@ Available checks are: type. Unlike signed integer overflow, this is not undefined behavior, but it is often unintentional. This sanitizer does not check for lossy implicit conversions performed before such a computation - (see ``-fsanitize=implicit-conversion``). + (see ``-fsanitize=implicit-integer-conversion``). - ``-fsanitize=vla-bound``: A variable-length array whose bound does not evaluate to a positive value. - ``-fsanitize=vptr``: Use of an object whose vptr indicates that it is of @@ -224,11 +229,15 @@ You can also use the following check groups: - ``-fsanitize=implicit-integer-arithmetic-value-change``: Catches implicit
[clang] [compiler-rt] Fix "[clang][UBSan] Add implicit conversion check for bitfields" (PR #87761)
https://github.com/Zonotora created
https://github.com/llvm/llvm-project/pull/87761
Fix since #75481 got reverted.
- Explicitly set BitfieldBits to 0 to avoid uninitialized field member for the
integer checks:
```diff
- llvm::ConstantInt::get(Builder.getInt8Ty(), Check.first)};
+ llvm::ConstantInt::get(Builder.getInt8Ty(), Check.first),
+ llvm::ConstantInt::get(Builder.getInt32Ty(), 0)};
```
- `Value **Previous` was erroneously `Value *Previous` in
`CodeGenFunction::EmitWithOriginalRHSBitfieldAssignment`, fixed now.
- Assert instead of check:
```diff
- if (Kind == CK_IntegralCast) {
+assert(Kind == CK_IntegralCast || Kind == CK_LValueToRValue &&
+ "Should be either CK_IntegralCast or CK_LValueToRValue");
```
CK_LValueToRValue when going from, e.g., char to char, and CK_IntegralCast
otherwise.
- Make sure that `Value *Previous = nullptr;` is initialized (see
https://github.com/llvm/llvm-project/commit/1189e87951e59a81ee097eae847c06008276fef1)
- Add another extensive testcase
`ubsan/TestCases/ImplicitConversion/bitfield-conversion.c`
>From c782d7d15788f87a850d29e770042f86c939185b Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Thu, 4 Apr 2024 20:27:14 +0200
Subject: [PATCH] Fix "[clang][UBSan] Add implicit conversion check for
bitfields"
---
clang/docs/ReleaseNotes.rst | 7 +
clang/docs/UndefinedBehaviorSanitizer.rst | 19 +-
clang/include/clang/Basic/Sanitizers.def | 20 +-
clang/lib/CodeGen/CGExpr.cpp | 37 +-
clang/lib/CodeGen/CGExprScalar.cpp| 264 +++-
clang/lib/CodeGen/CodeGenFunction.h | 15 +
.../test/CodeGen/ubsan-bitfield-conversion.c | 61 ++
.../CodeGenCXX/ubsan-bitfield-conversion.cpp | 94 +++
clang/test/Driver/fsanitize.c | 28 +-
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 27 +-
compiler-rt/lib/ubsan/ubsan_handlers.h| 1 +
.../ImplicitConversion/bitfield-conversion.c | 641 ++
12 files changed, 1139 insertions(+), 75 deletions(-)
create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c
create mode 100644 clang/test/CodeGenCXX/ubsan-bitfield-conversion.cpp
create mode 100644
compiler-rt/test/ubsan/TestCases/ImplicitConversion/bitfield-conversion.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 28e8ddb3c41c3e9..abf15c8dc49d9e7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -198,6 +198,10 @@ Non-comprehensive list of changes in this release
New Compiler Flags
--
+- ``-fsanitize=implicit-bitfield-conversion`` checks implicit truncation and
+ sign change.
+- ``-fsanitize=implicit-integer-conversion`` a group that replaces the previous
+ group ``-fsanitize=implicit-conversion``.
- ``-Wmissing-designated-field-initializers``, grouped under
``-Wmissing-field-initializers``.
This diagnostic can be disabled to make ``-Wmissing-field-initializers``
behave
@@ -211,6 +215,9 @@ Modified Compiler Flags
- Added a new diagnostic flag ``-Wreturn-mismatch`` which is grouped under
``-Wreturn-type``, and moved some of the diagnostics previously controlled by
``-Wreturn-type`` under this new flag. Fixes #GH72116.
+- ``-fsanitize=implicit-conversion`` is now a group for both
+ ``-fsanitize=implicit-integer-conversion`` and
+ ``-fsanitize=implicit-bitfield-conversion``.
- Added ``-Wcast-function-type-mismatch`` under the ``-Wcast-function-type``
warning group. Moved the diagnostic previously controlled by
diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst
b/clang/docs/UndefinedBehaviorSanitizer.rst
index 8f58c92bd2a1634..531d56e313826c7 100644
--- a/clang/docs/UndefinedBehaviorSanitizer.rst
+++ b/clang/docs/UndefinedBehaviorSanitizer.rst
@@ -148,6 +148,11 @@ Available checks are:
Issues caught by this sanitizer are not undefined behavior,
but are often unintentional.
- ``-fsanitize=integer-divide-by-zero``: Integer division by zero.
+ - ``-fsanitize=implicit-bitfield-conversion``: Implicit conversion from
+ integer of larger bit width to smaller bitfield, if that results in data
+ loss. This includes unsigned/signed truncations and sign changes,
similarly
+ to how the ``-fsanitize=implicit-integer-conversion`` group works, but
+ explicitly for bitfields.
- ``-fsanitize=nonnull-attribute``: Passing null pointer as a function
parameter which is declared to never be null.
- ``-fsanitize=null``: Use of a null pointer or creation of a null
@@ -193,8 +198,8 @@ Available checks are:
signed division overflow (``INT_MIN/-1``). Note that checks are still
added even when ``-fwrapv`` is enabled. This sanitizer does not check for
lossy implicit conversions performed before the computation (see
- ``-fsanitize=implicit-conversion``). Both of these two issues are handled
- by ``-fsanitize=implicit-conversion`` group of checks.
+ ``-fsanitize=implicit-intege
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
Zonotora wrote: > Also, I don't have commit access (first PR here), someone has to commit on my > behalf! @AaronBallman https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
Zonotora wrote: Also, I don't have commit access (first PR here), someone has to commit on my behalf! https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
https://github.com/Zonotora updated
https://github.com/llvm/llvm-project/pull/75481
>From 42207b6369b22db7e8400f290be8c6ee75a5278a Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:33:21 +0100
Subject: [PATCH 1/3] [clang] Extract negativity check lambda to function
---
clang/lib/CodeGen/CGExprScalar.cpp | 41 ++
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index 8536570087ad0f..74d94259572dc2 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -1103,6 +1103,21 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
+ const char *Name,
+ CGBuilderTy &Builder) {
+ bool VSigned = VType->isSignedIntegerOrEnumerationType();
+ llvm::Type *VTy = V->getType();
+ if (!VSigned) {
+// If the value is unsigned, then it is never negative.
+return llvm::ConstantInt::getFalse(VTy->getContext());
+ }
+ llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
+ return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
+llvm::Twine(Name) + "." + V->getName() +
+".negativitycheck");
+};
+
// Should be called within CodeGenFunction::SanitizerScope RAII scope.
// Returns 'i1 false' when the conversion Src -> Dst changed the sign.
static std::pair Value * {
-// Is this value a signed type?
-bool VSigned = VType->isSignedIntegerOrEnumerationType();
-llvm::Type *VTy = V->getType();
-if (!VSigned) {
- // If the value is unsigned, then it is never negative.
- // FIXME: can we encounter non-scalar VTy here?
- return llvm::ConstantInt::getFalse(VTy->getContext());
-}
-// Get the zero of the same type with which we will be comparing.
-llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
-// %V.isnegative = icmp slt %V, 0
-// I.e is %V *strictly* less than zero, does it have negative value?
-return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
- llvm::Twine(Name) + "." + V->getName() +
- ".negativitycheck");
- };
-
// 1. Was the old Value negative?
- llvm::Value *SrcIsNegative = EmitIsNegativeTest(Src, SrcType, "src");
+ llvm::Value *SrcIsNegative =
+ EmitIsNegativeTestHelper(Src, SrcType, "src", Builder);
// 2. Is the new Value negative?
- llvm::Value *DstIsNegative = EmitIsNegativeTest(Dst, DstType, "dst");
+ llvm::Value *DstIsNegative =
+ EmitIsNegativeTestHelper(Dst, DstType, "dst", Builder);
// 3. Now, was the 'negativity status' preserved during the conversion?
//NOTE: conversion from negative to zero is considered to change the
sign.
//(We want to get 'false' when the conversion changed the sign)
>From 77fed882c1d6bbe4cbe16feea337df0b9c3f83cf Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:36:04 +0100
Subject: [PATCH 2/3] [clang][UBSan] Add implicit conversion check for
bitfields
This patch implements the implicit truncation and implicit
sign change checks for bitfields.
---
clang/docs/ReleaseNotes.rst | 7 +
clang/docs/UndefinedBehaviorSanitizer.rst | 19 +-
clang/include/clang/Basic/Sanitizers.def | 20 +-
clang/lib/CodeGen/CGExpr.cpp | 37 ++-
clang/lib/CodeGen/CGExprScalar.cpp| 218 +-
clang/lib/CodeGen/CodeGenFunction.h | 15 ++
.../test/CodeGen/ubsan-bitfield-conversion.c | 61 +
.../CodeGenCXX/ubsan-bitfield-conversion.cpp | 94
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 27 ++-
compiler-rt/lib/ubsan/ubsan_handlers.h| 1 +
10 files changed, 461 insertions(+), 38 deletions(-)
create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c
create mode 100644 clang/test/CodeGenCXX/ubsan-bitfield-conversion.cpp
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 125d51c42d507f..0e72e519c9018c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -179,6 +179,10 @@ Non-comprehensive list of changes in this release
New Compiler Flags
--
+- ``-fsanitize=implicit-bitfield-conversion`` checks implicit truncation and
+ sign change.
+- ``-fsanitize=implicit-integer-conversion`` a group that replaces the previous
+ group ``-fsanitize=implicit-conversion``.
- ``-Wmissing-designated-field-initializers``, grouped under
``-Wmissing-field-initializers``.
This diagnostic can be disabled to make ``-Wmissing-field-initializers``
behave
@@ -192,6 +196,9 @@ Modified Compiler Flags
- Added a new diagnostic flag ``-Wreturn-mismatch`` which is grouped under
``-Wreturn-type``, and mo
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
Zonotora wrote: @LebedevRI LGTM? https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
@@ -0,0 +1,61 @@ +// RUN: %clang -fsanitize=implicit-bitfield-conversion -target x86_64-linux -S -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK-BITFIELD-CONVERSION Zonotora wrote: Added! https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
https://github.com/Zonotora updated
https://github.com/llvm/llvm-project/pull/75481
>From 3213a5d3c8230cc941d026475624b754bc87a41e Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:33:21 +0100
Subject: [PATCH 1/3] [clang] Extract negativity check lambda to function
---
clang/lib/CodeGen/CGExprScalar.cpp | 41 ++
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index 8536570087ad0f..74d94259572dc2 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -1103,6 +1103,21 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
+ const char *Name,
+ CGBuilderTy &Builder) {
+ bool VSigned = VType->isSignedIntegerOrEnumerationType();
+ llvm::Type *VTy = V->getType();
+ if (!VSigned) {
+// If the value is unsigned, then it is never negative.
+return llvm::ConstantInt::getFalse(VTy->getContext());
+ }
+ llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
+ return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
+llvm::Twine(Name) + "." + V->getName() +
+".negativitycheck");
+};
+
// Should be called within CodeGenFunction::SanitizerScope RAII scope.
// Returns 'i1 false' when the conversion Src -> Dst changed the sign.
static std::pair Value * {
-// Is this value a signed type?
-bool VSigned = VType->isSignedIntegerOrEnumerationType();
-llvm::Type *VTy = V->getType();
-if (!VSigned) {
- // If the value is unsigned, then it is never negative.
- // FIXME: can we encounter non-scalar VTy here?
- return llvm::ConstantInt::getFalse(VTy->getContext());
-}
-// Get the zero of the same type with which we will be comparing.
-llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
-// %V.isnegative = icmp slt %V, 0
-// I.e is %V *strictly* less than zero, does it have negative value?
-return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
- llvm::Twine(Name) + "." + V->getName() +
- ".negativitycheck");
- };
-
// 1. Was the old Value negative?
- llvm::Value *SrcIsNegative = EmitIsNegativeTest(Src, SrcType, "src");
+ llvm::Value *SrcIsNegative =
+ EmitIsNegativeTestHelper(Src, SrcType, "src", Builder);
// 2. Is the new Value negative?
- llvm::Value *DstIsNegative = EmitIsNegativeTest(Dst, DstType, "dst");
+ llvm::Value *DstIsNegative =
+ EmitIsNegativeTestHelper(Dst, DstType, "dst", Builder);
// 3. Now, was the 'negativity status' preserved during the conversion?
//NOTE: conversion from negative to zero is considered to change the
sign.
//(We want to get 'false' when the conversion changed the sign)
>From 0cf83f5f9cb9021c57d84f3e9e3ed1f1b0bb26b5 Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:36:04 +0100
Subject: [PATCH 2/3] [clang][UBSan] Add implicit conversion check for
bitfields
This patch implements the implicit truncation and implicit
sign change checks for bitfields.
---
clang/docs/ReleaseNotes.rst | 7 +
clang/docs/UndefinedBehaviorSanitizer.rst | 19 +-
clang/include/clang/Basic/Sanitizers.def | 20 +-
clang/lib/CodeGen/CGExpr.cpp | 37 ++-
clang/lib/CodeGen/CGExprScalar.cpp| 218 +-
clang/lib/CodeGen/CodeGenFunction.h | 15 ++
.../test/CodeGen/ubsan-bitfield-conversion.c | 61 +
.../CodeGenCXX/ubsan-bitfield-conversion.cpp | 94
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 27 ++-
compiler-rt/lib/ubsan/ubsan_handlers.h| 1 +
10 files changed, 461 insertions(+), 38 deletions(-)
create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c
create mode 100644 clang/test/CodeGenCXX/ubsan-bitfield-conversion.cpp
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 459f6a25aeef7b..312ac5483e94b9 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -173,6 +173,10 @@ Non-comprehensive list of changes in this release
New Compiler Flags
--
+- ``-fsanitize=implicit-bitfield-conversion`` checks implicit truncation and
+ sign change.
+- ``-fsanitize=implicit-integer-conversion`` a group that replaces the previous
+ group ``-fsanitize=implicit-conversion``.
- ``-Wmissing-designated-field-initializers``, grouped under
``-Wmissing-field-initializers``.
This diagnostic can be disabled to make ``-Wmissing-field-initializers``
behave
@@ -186,6 +190,9 @@ Modified Compiler Flags
- Added a new diagnostic flag ``-Wreturn-mismatch`` which is grouped under
``-Wreturn-type``, and mo
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
@@ -147,6 +147,7 @@ struct ImplicitConversionData {
const TypeDescriptor &FromType;
const TypeDescriptor &ToType;
/* ImplicitConversionCheckKind */ unsigned char Kind;
+ unsigned int BitfieldBits;
Zonotora wrote:
@zygoloid
https://github.com/llvm/llvm-project/pull/75481
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
@@ -0,0 +1,61 @@ +// RUN: %clang -fsanitize=implicit-bitfield-conversion -target x86_64-linux -S -emit-llvm -o - %s | FileCheck %s --check-prefixes=CHECK,CHECK-BITFIELD-CONVERSION Zonotora wrote: I will add some testcases later today! https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
https://github.com/Zonotora updated
https://github.com/llvm/llvm-project/pull/75481
>From f0f02479736548a72c8a35ca3fea067b02e66bfd Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:33:21 +0100
Subject: [PATCH 1/3] [clang] Extract negativity check lambda to function
---
clang/lib/CodeGen/CGExprScalar.cpp | 41 ++
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index 8536570087ad0f..74d94259572dc2 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -1103,6 +1103,21 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
+ const char *Name,
+ CGBuilderTy &Builder) {
+ bool VSigned = VType->isSignedIntegerOrEnumerationType();
+ llvm::Type *VTy = V->getType();
+ if (!VSigned) {
+// If the value is unsigned, then it is never negative.
+return llvm::ConstantInt::getFalse(VTy->getContext());
+ }
+ llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
+ return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
+llvm::Twine(Name) + "." + V->getName() +
+".negativitycheck");
+};
+
// Should be called within CodeGenFunction::SanitizerScope RAII scope.
// Returns 'i1 false' when the conversion Src -> Dst changed the sign.
static std::pair Value * {
-// Is this value a signed type?
-bool VSigned = VType->isSignedIntegerOrEnumerationType();
-llvm::Type *VTy = V->getType();
-if (!VSigned) {
- // If the value is unsigned, then it is never negative.
- // FIXME: can we encounter non-scalar VTy here?
- return llvm::ConstantInt::getFalse(VTy->getContext());
-}
-// Get the zero of the same type with which we will be comparing.
-llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
-// %V.isnegative = icmp slt %V, 0
-// I.e is %V *strictly* less than zero, does it have negative value?
-return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
- llvm::Twine(Name) + "." + V->getName() +
- ".negativitycheck");
- };
-
// 1. Was the old Value negative?
- llvm::Value *SrcIsNegative = EmitIsNegativeTest(Src, SrcType, "src");
+ llvm::Value *SrcIsNegative =
+ EmitIsNegativeTestHelper(Src, SrcType, "src", Builder);
// 2. Is the new Value negative?
- llvm::Value *DstIsNegative = EmitIsNegativeTest(Dst, DstType, "dst");
+ llvm::Value *DstIsNegative =
+ EmitIsNegativeTestHelper(Dst, DstType, "dst", Builder);
// 3. Now, was the 'negativity status' preserved during the conversion?
//NOTE: conversion from negative to zero is considered to change the
sign.
//(We want to get 'false' when the conversion changed the sign)
>From 99e461390a8d64a416a61e27a5150b41e0baad34 Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:36:04 +0100
Subject: [PATCH 2/3] [clang][UBSan] Add implicit conversion check for
bitfields
This patch implements the implicit truncation and implicit
sign change checks for bitfields.
---
clang/docs/ReleaseNotes.rst | 7 +
clang/docs/UndefinedBehaviorSanitizer.rst | 19 +-
clang/include/clang/Basic/Sanitizers.def | 20 +-
clang/lib/CodeGen/CGExpr.cpp | 37 ++-
clang/lib/CodeGen/CGExprScalar.cpp| 218 +-
clang/lib/CodeGen/CodeGenFunction.h | 15 ++
.../test/CodeGen/ubsan-bitfield-conversion.c | 61 +
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 27 ++-
compiler-rt/lib/ubsan/ubsan_handlers.h| 1 +
9 files changed, 367 insertions(+), 38 deletions(-)
create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e14c92eae0afe1..e07019f4a86427 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -173,6 +173,10 @@ Non-comprehensive list of changes in this release
New Compiler Flags
--
+- ``-fsanitize=implicit-bitfield-conversion`` checks implicit truncation and
+ sign change.
+- ``-fsanitize=implicit-integer-conversion`` a group that replaces the previous
+ group ``-fsanitize=implicit-conversion``.
- ``-Wmissing-designated-field-initializers``, grouped under
``-Wmissing-field-initializers``.
This diagnostic can be disabled to make ``-Wmissing-field-initializers``
behave
@@ -186,6 +190,9 @@ Modified Compiler Flags
- Added a new diagnostic flag ``-Wreturn-mismatch`` which is grouped under
``-Wreturn-type``, and moved some of the diagnostics previously controlled by
``-Wreturn-type`` under this new flag. Fixes #GH72116.
+- ``-fsanitize=implicit
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
https://github.com/Zonotora updated
https://github.com/llvm/llvm-project/pull/75481
>From cd64144a4cf18adba1494871bfd9da1b0bf0a489 Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:33:21 +0100
Subject: [PATCH 1/3] [clang] Extract negativity check lambda to function
---
clang/lib/CodeGen/CGExprScalar.cpp | 41 ++
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index 8536570087ad0f..74d94259572dc2 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -1103,6 +1103,21 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
+ const char *Name,
+ CGBuilderTy &Builder) {
+ bool VSigned = VType->isSignedIntegerOrEnumerationType();
+ llvm::Type *VTy = V->getType();
+ if (!VSigned) {
+// If the value is unsigned, then it is never negative.
+return llvm::ConstantInt::getFalse(VTy->getContext());
+ }
+ llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
+ return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
+llvm::Twine(Name) + "." + V->getName() +
+".negativitycheck");
+};
+
// Should be called within CodeGenFunction::SanitizerScope RAII scope.
// Returns 'i1 false' when the conversion Src -> Dst changed the sign.
static std::pair Value * {
-// Is this value a signed type?
-bool VSigned = VType->isSignedIntegerOrEnumerationType();
-llvm::Type *VTy = V->getType();
-if (!VSigned) {
- // If the value is unsigned, then it is never negative.
- // FIXME: can we encounter non-scalar VTy here?
- return llvm::ConstantInt::getFalse(VTy->getContext());
-}
-// Get the zero of the same type with which we will be comparing.
-llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
-// %V.isnegative = icmp slt %V, 0
-// I.e is %V *strictly* less than zero, does it have negative value?
-return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
- llvm::Twine(Name) + "." + V->getName() +
- ".negativitycheck");
- };
-
// 1. Was the old Value negative?
- llvm::Value *SrcIsNegative = EmitIsNegativeTest(Src, SrcType, "src");
+ llvm::Value *SrcIsNegative =
+ EmitIsNegativeTestHelper(Src, SrcType, "src", Builder);
// 2. Is the new Value negative?
- llvm::Value *DstIsNegative = EmitIsNegativeTest(Dst, DstType, "dst");
+ llvm::Value *DstIsNegative =
+ EmitIsNegativeTestHelper(Dst, DstType, "dst", Builder);
// 3. Now, was the 'negativity status' preserved during the conversion?
//NOTE: conversion from negative to zero is considered to change the
sign.
//(We want to get 'false' when the conversion changed the sign)
>From da992660e5e7cc48aaef904dbbf3035138cb0f75 Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:36:04 +0100
Subject: [PATCH 2/3] [clang][UBSan] Add implicit conversion check for
bitfields
This patch implements the implicit truncation and implicit
sign change checks for bitfields.
---
clang/docs/ReleaseNotes.rst | 7 +
clang/docs/UndefinedBehaviorSanitizer.rst | 19 +-
clang/include/clang/Basic/Sanitizers.def | 20 +-
clang/lib/CodeGen/CGExpr.cpp | 37 ++-
clang/lib/CodeGen/CGExprScalar.cpp| 218 +-
clang/lib/CodeGen/CodeGenFunction.h | 15 ++
.../test/CodeGen/ubsan-bitfield-conversion.c | 61 +
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 27 ++-
compiler-rt/lib/ubsan/ubsan_handlers.h| 1 +
9 files changed, 367 insertions(+), 38 deletions(-)
create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 3b89d5a8720785..7a8135077a2bc2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -173,6 +173,10 @@ Non-comprehensive list of changes in this release
New Compiler Flags
--
+- ``-fsanitize=implicit-bitfield-conversion`` checks implicit truncation and
+ sign change.
+- ``-fsanitize=implicit-integer-conversion`` a group that replaces the previous
+ group ``-fsanitize=implicit-conversion``.
- ``-Wmissing-designated-field-initializers``, grouped under
``-Wmissing-field-initializers``.
This diagnostic can be disabled to make ``-Wmissing-field-initializers``
behave
@@ -183,6 +187,9 @@ Deprecated Compiler Flags
Modified Compiler Flags
---
+- ``-fsanitize=implicit-conversion`` is now a group for both
+ ``-fsanitize=implicit-integer-conversion`` and
+ ``-fsanitize=implicit-bitfield-conversion``.
Removed Compiler Flags
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
@@ -5571,11 +5571,52 @@ LValue CodeGenFunction::EmitBinaryOperatorLValue(const
BinaryOperator *E) {
break;
}
-RValue RV = EmitAnyExpr(E->getRHS());
+llvm::Value *Previous = nullptr;
+RValue RV;
+QualType SrcType = E->getRHS()->getType();
+// Check if LHS is a bitfield, if RHS contains an implicit cast expression
+// we want to extract that value and potentially (if the bitfield sanitizer
+// is enabled) use it to check for an implicit conversion.
+if (E->getLHS()->refersToBitField()) {
+ // Get the RHS before scalar conversion.
+ if (auto *ICE = GetOriginalRHSForBitfieldSanitizer(E)) {
Zonotora wrote:
Will do!
https://github.com/llvm/llvm-project/pull/75481
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
@@ -5571,11 +5571,52 @@ LValue CodeGenFunction::EmitBinaryOperatorLValue(const
BinaryOperator *E) {
break;
}
-RValue RV = EmitAnyExpr(E->getRHS());
+llvm::Value *Previous = nullptr;
Zonotora wrote:
I would also like to have it work similar to how EmitCompoundAssignmentLValue
works, however it doesn't seem completely trivial to make this change? Maybe it
is better suited in its own PR as well? But I can definitely convert:
```cpp
if (E->getLHS()->refersToBitField()) {
// Get the RHS before scalar conversion.
if (auto *ICE = GetOriginalRHSForBitfieldAssignment(E)) {
SrcType = ICE->getSubExpr()->getType();
Previous = EmitScalarExpr(ICE->getSubExpr());
// Pass default ScalarConversionOpts to avoid emitting
// integer sanitizer checks as E refers to bitfield.
llvm::Value *RHS = EmitScalarConversion(
Previous, SrcType, ICE->getType(), ICE->getExprLoc());
RV = RValue::get(RHS);
}
}
```
to something like:
```cpp
if (E->getLHS()->refersToBitField())
Previous = EmitForOriginalRHSBitfieldAssignment(E, &RHS));
```
to at least de-duplicate some parts of it...
https://github.com/llvm/llvm-project/pull/75481
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
Zonotora wrote: @zygoloid Thanks for your feedback! Fixed all comments except the last one. I totally agree with you about the distinction of UB checks, and that should be part of a different future PR! https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
@@ -555,13 +555,11 @@ static void
handleImplicitConversion(ImplicitConversionData *Data,
ReportOptions Opts, ValueHandle Src,
ValueHandle Dst) {
SourceLocation Loc = Data->Loc.acquire();
- ErrorType ET = ErrorType::GenericUB;
-
const TypeDescriptor &SrcTy = Data->FromType;
const TypeDescriptor &DstTy = Data->ToType;
-
bool SrcSigned = SrcTy.isSignedIntegerTy();
bool DstSigned = DstTy.isSignedIntegerTy();
+ ErrorType ET = ErrorType::GenericUB;
Zonotora wrote:
I have only moved this line... I think changing the error type is for a future
PR as you comment further down.
https://github.com/llvm/llvm-project/pull/75481
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
https://github.com/Zonotora updated
https://github.com/llvm/llvm-project/pull/75481
>From cd64144a4cf18adba1494871bfd9da1b0bf0a489 Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:33:21 +0100
Subject: [PATCH 1/3] [clang] Extract negativity check lambda to function
---
clang/lib/CodeGen/CGExprScalar.cpp | 41 ++
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index 8536570087ad0f..74d94259572dc2 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -1103,6 +1103,21 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
+ const char *Name,
+ CGBuilderTy &Builder) {
+ bool VSigned = VType->isSignedIntegerOrEnumerationType();
+ llvm::Type *VTy = V->getType();
+ if (!VSigned) {
+// If the value is unsigned, then it is never negative.
+return llvm::ConstantInt::getFalse(VTy->getContext());
+ }
+ llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
+ return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
+llvm::Twine(Name) + "." + V->getName() +
+".negativitycheck");
+};
+
// Should be called within CodeGenFunction::SanitizerScope RAII scope.
// Returns 'i1 false' when the conversion Src -> Dst changed the sign.
static std::pair Value * {
-// Is this value a signed type?
-bool VSigned = VType->isSignedIntegerOrEnumerationType();
-llvm::Type *VTy = V->getType();
-if (!VSigned) {
- // If the value is unsigned, then it is never negative.
- // FIXME: can we encounter non-scalar VTy here?
- return llvm::ConstantInt::getFalse(VTy->getContext());
-}
-// Get the zero of the same type with which we will be comparing.
-llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
-// %V.isnegative = icmp slt %V, 0
-// I.e is %V *strictly* less than zero, does it have negative value?
-return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
- llvm::Twine(Name) + "." + V->getName() +
- ".negativitycheck");
- };
-
// 1. Was the old Value negative?
- llvm::Value *SrcIsNegative = EmitIsNegativeTest(Src, SrcType, "src");
+ llvm::Value *SrcIsNegative =
+ EmitIsNegativeTestHelper(Src, SrcType, "src", Builder);
// 2. Is the new Value negative?
- llvm::Value *DstIsNegative = EmitIsNegativeTest(Dst, DstType, "dst");
+ llvm::Value *DstIsNegative =
+ EmitIsNegativeTestHelper(Dst, DstType, "dst", Builder);
// 3. Now, was the 'negativity status' preserved during the conversion?
//NOTE: conversion from negative to zero is considered to change the
sign.
//(We want to get 'false' when the conversion changed the sign)
>From e5914b0519efa0d70c429f2b08b326f863df4fbe Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:36:04 +0100
Subject: [PATCH 2/3] [clang][UBSan] Add implicit conversion check for
bitfields
This patch implements the implicit truncation and implicit
sign change checks for bitfields.
---
clang/docs/ReleaseNotes.rst | 7 +
clang/docs/UndefinedBehaviorSanitizer.rst | 19 +-
clang/include/clang/Basic/Sanitizers.def | 20 +-
clang/lib/CodeGen/CGExpr.cpp | 45 +++-
clang/lib/CodeGen/CGExprScalar.cpp| 224 +-
clang/lib/CodeGen/CodeGenFunction.h | 12 +
.../test/CodeGen/ubsan-bitfield-conversion.c | 61 +
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 27 ++-
compiler-rt/lib/ubsan/ubsan_handlers.h| 1 +
9 files changed, 378 insertions(+), 38 deletions(-)
create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 3b89d5a8720785..7a8135077a2bc2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -173,6 +173,10 @@ Non-comprehensive list of changes in this release
New Compiler Flags
--
+- ``-fsanitize=implicit-bitfield-conversion`` checks implicit truncation and
+ sign change.
+- ``-fsanitize=implicit-integer-conversion`` a group that replaces the previous
+ group ``-fsanitize=implicit-conversion``.
- ``-Wmissing-designated-field-initializers``, grouped under
``-Wmissing-field-initializers``.
This diagnostic can be disabled to make ``-Wmissing-field-initializers``
behave
@@ -183,6 +187,9 @@ Deprecated Compiler Flags
Modified Compiler Flags
---
+- ``-fsanitize=implicit-conversion`` is now a group for both
+ ``-fsanitize=implicit-integer-conversion`` and
+ ``-fsanitize=implicit-bitfield-conversion``.
Removed Compiler Flags
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
Zonotora wrote: @zygoloid @AaronBallman any updates? https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
Zonotora wrote: > Precommit CI seems to have found some related failures that should be > investigated. I'd love to hear opinions from the codegen code owners on this, > as well as @zygoloid as UBSan code owner. I'm not opposed to the changes, > though I do find it somewhat strange to add them to UBSan given that they're > not undefined behavior (even though there's precedent). @AaronBallman CI Linux is successful, but "Trigger build on clang-ci" seems to be failing on unrelated format and windows build? I also find it strange that these checks are included in UBSan. However, it might be better to refactor this in a future PR, as you say, there are reasons to include it in UBSan now because of the current `-fsanitize=implicit-conversion` flag. How do we move this forward and merge the PR? https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
Zonotora wrote: @zygoloid what do you think? https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
https://github.com/Zonotora updated
https://github.com/llvm/llvm-project/pull/75481
>From a3de13f8356cd615534c759b6e24893d361e62e9 Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:33:21 +0100
Subject: [PATCH 1/3] [clang] Extract negativity check lambda to function
---
clang/lib/CodeGen/CGExprScalar.cpp | 41 ++
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index 576734e460b9c1..438645ac0ab374 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -1097,6 +1097,21 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
+ const char *Name,
+ CGBuilderTy &Builder) {
+ bool VSigned = VType->isSignedIntegerOrEnumerationType();
+ llvm::Type *VTy = V->getType();
+ if (!VSigned) {
+// If the value is unsigned, then it is never negative.
+return llvm::ConstantInt::getFalse(VTy->getContext());
+ }
+ llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
+ return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
+llvm::Twine(Name) + "." + V->getName() +
+".negativitycheck");
+};
+
// Should be called within CodeGenFunction::SanitizerScope RAII scope.
// Returns 'i1 false' when the conversion Src -> Dst changed the sign.
static std::pair Value * {
-// Is this value a signed type?
-bool VSigned = VType->isSignedIntegerOrEnumerationType();
-llvm::Type *VTy = V->getType();
-if (!VSigned) {
- // If the value is unsigned, then it is never negative.
- // FIXME: can we encounter non-scalar VTy here?
- return llvm::ConstantInt::getFalse(VTy->getContext());
-}
-// Get the zero of the same type with which we will be comparing.
-llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
-// %V.isnegative = icmp slt %V, 0
-// I.e is %V *strictly* less than zero, does it have negative value?
-return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
- llvm::Twine(Name) + "." + V->getName() +
- ".negativitycheck");
- };
-
// 1. Was the old Value negative?
- llvm::Value *SrcIsNegative = EmitIsNegativeTest(Src, SrcType, "src");
+ llvm::Value *SrcIsNegative =
+ EmitIsNegativeTestHelper(Src, SrcType, "src", Builder);
// 2. Is the new Value negative?
- llvm::Value *DstIsNegative = EmitIsNegativeTest(Dst, DstType, "dst");
+ llvm::Value *DstIsNegative =
+ EmitIsNegativeTestHelper(Dst, DstType, "dst", Builder);
// 3. Now, was the 'negativity status' preserved during the conversion?
//NOTE: conversion from negative to zero is considered to change the
sign.
//(We want to get 'false' when the conversion changed the sign)
>From e7f09c4cc8a45b5517e02eee2045820ff77c5210 Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:36:04 +0100
Subject: [PATCH 2/3] [clang][UBSan] Add implicit conversion check for
bitfields
This patch implements the implicit truncation and implicit
sign change checks for bitfields.
---
clang/docs/ReleaseNotes.rst | 7 +
clang/docs/UndefinedBehaviorSanitizer.rst | 19 +-
clang/include/clang/Basic/Sanitizers.def | 20 +-
clang/lib/CodeGen/CGExpr.cpp | 43 +++-
clang/lib/CodeGen/CGExprScalar.cpp| 224 +-
clang/lib/CodeGen/CodeGenFunction.h | 12 +
.../test/CodeGen/ubsan-bitfield-conversion.c | 61 +
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 27 ++-
compiler-rt/lib/ubsan/ubsan_handlers.h| 1 +
9 files changed, 376 insertions(+), 38 deletions(-)
create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 5bca2c965c866b..504405362de3d2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -157,6 +157,10 @@ Non-comprehensive list of changes in this release
New Compiler Flags
--
+- ``-fsanitize=implicit-bitfield-conversion`` checks implicit truncation and
+ sign change.
+- ``-fsanitize=implicit-integer-conversion`` a group that replaces the previous
+ group ``-fsanitize=implicit-conversion``.
.. _target_os_detail:
@@ -174,6 +178,9 @@ Deprecated Compiler Flags
Modified Compiler Flags
---
+- ``-fsanitize=implicit-conversion`` is now a group for both
+ ``-fsanitize=implicit-integer-conversion`` and
+ ``-fsanitize=implicit-bitfield-conversion``.
Removed Compiler Flags
-
diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst
b/clang/docs/UndefinedBehaviorSanitizer.rst
index b8ad3804f18903..eca82e
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
Zonotora wrote: @rjmccall fixed feedback > You should handle compound assignments. I believe I already support compound assignments (and PrePostIncDec), if I am not missing something? :smiley: https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
https://github.com/Zonotora updated
https://github.com/llvm/llvm-project/pull/75481
>From 5c85a13cbd5fb1d861f2b5a4cbb53abc5736674e Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:33:21 +0100
Subject: [PATCH 1/3] [clang] Extract negativity check lambda to function
---
clang/lib/CodeGen/CGExprScalar.cpp | 41 ++
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index aa805f291d1757..1af89535d8cd76 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -1097,6 +1097,21 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
+ const char *Name,
+ CGBuilderTy &Builder) {
+ bool VSigned = VType->isSignedIntegerOrEnumerationType();
+ llvm::Type *VTy = V->getType();
+ if (!VSigned) {
+// If the value is unsigned, then it is never negative.
+return llvm::ConstantInt::getFalse(VTy->getContext());
+ }
+ llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
+ return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
+llvm::Twine(Name) + "." + V->getName() +
+".negativitycheck");
+};
+
// Should be called within CodeGenFunction::SanitizerScope RAII scope.
// Returns 'i1 false' when the conversion Src -> Dst changed the sign.
static std::pair Value * {
-// Is this value a signed type?
-bool VSigned = VType->isSignedIntegerOrEnumerationType();
-llvm::Type *VTy = V->getType();
-if (!VSigned) {
- // If the value is unsigned, then it is never negative.
- // FIXME: can we encounter non-scalar VTy here?
- return llvm::ConstantInt::getFalse(VTy->getContext());
-}
-// Get the zero of the same type with which we will be comparing.
-llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
-// %V.isnegative = icmp slt %V, 0
-// I.e is %V *strictly* less than zero, does it have negative value?
-return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
- llvm::Twine(Name) + "." + V->getName() +
- ".negativitycheck");
- };
-
// 1. Was the old Value negative?
- llvm::Value *SrcIsNegative = EmitIsNegativeTest(Src, SrcType, "src");
+ llvm::Value *SrcIsNegative =
+ EmitIsNegativeTestHelper(Src, SrcType, "src", Builder);
// 2. Is the new Value negative?
- llvm::Value *DstIsNegative = EmitIsNegativeTest(Dst, DstType, "dst");
+ llvm::Value *DstIsNegative =
+ EmitIsNegativeTestHelper(Dst, DstType, "dst", Builder);
// 3. Now, was the 'negativity status' preserved during the conversion?
//NOTE: conversion from negative to zero is considered to change the
sign.
//(We want to get 'false' when the conversion changed the sign)
>From b8afc478615b06a7f7d9c8adbe9ed0ffb9d89c13 Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:36:04 +0100
Subject: [PATCH 2/3] [clang][UBSan] Add implicit conversion check for
bitfields
This patch implements the implicit truncation and implicit
sign change checks for bitfields.
---
clang/docs/ReleaseNotes.rst | 7 +
clang/docs/UndefinedBehaviorSanitizer.rst | 19 +-
clang/include/clang/Basic/Sanitizers.def | 20 +-
clang/lib/CodeGen/CGExpr.cpp | 43 +++-
clang/lib/CodeGen/CGExprScalar.cpp| 224 +-
clang/lib/CodeGen/CodeGenFunction.h | 12 +
.../test/CodeGen/ubsan-bitfield-conversion.c | 32 +++
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 27 ++-
compiler-rt/lib/ubsan/ubsan_handlers.h| 1 +
9 files changed, 347 insertions(+), 38 deletions(-)
create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b5e6eae9f1bf7d..934be58cd1b9c4 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -143,6 +143,10 @@ Non-comprehensive list of changes in this release
New Compiler Flags
--
+- ``-fsanitize=implicit-bitfield-conversion`` checks implicit truncation and
+ sign change.
+- ``-fsanitize=implicit-integer-conversion`` a group that replaces the previous
+ group ``-fsanitize=implicit-conversion``.
.. _target_os_detail:
@@ -160,6 +164,9 @@ Deprecated Compiler Flags
Modified Compiler Flags
---
+- ``-fsanitize=implicit-conversion`` is now a group for both
+ ``-fsanitize=implicit-integer-conversion`` and
+ ``-fsanitize=implicit-bitfield-conversion``.
Removed Compiler Flags
-
diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst
b/clang/docs/UndefinedBehaviorSanitizer.rst
index b8ad3804f18903..eca82e1b
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
https://github.com/Zonotora updated
https://github.com/llvm/llvm-project/pull/75481
>From 5c85a13cbd5fb1d861f2b5a4cbb53abc5736674e Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:33:21 +0100
Subject: [PATCH 1/3] [clang] Extract negativity check lambda to function
---
clang/lib/CodeGen/CGExprScalar.cpp | 41 ++
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index aa805f291d1757..1af89535d8cd76 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -1097,6 +1097,21 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
+ const char *Name,
+ CGBuilderTy &Builder) {
+ bool VSigned = VType->isSignedIntegerOrEnumerationType();
+ llvm::Type *VTy = V->getType();
+ if (!VSigned) {
+// If the value is unsigned, then it is never negative.
+return llvm::ConstantInt::getFalse(VTy->getContext());
+ }
+ llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
+ return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
+llvm::Twine(Name) + "." + V->getName() +
+".negativitycheck");
+};
+
// Should be called within CodeGenFunction::SanitizerScope RAII scope.
// Returns 'i1 false' when the conversion Src -> Dst changed the sign.
static std::pair Value * {
-// Is this value a signed type?
-bool VSigned = VType->isSignedIntegerOrEnumerationType();
-llvm::Type *VTy = V->getType();
-if (!VSigned) {
- // If the value is unsigned, then it is never negative.
- // FIXME: can we encounter non-scalar VTy here?
- return llvm::ConstantInt::getFalse(VTy->getContext());
-}
-// Get the zero of the same type with which we will be comparing.
-llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
-// %V.isnegative = icmp slt %V, 0
-// I.e is %V *strictly* less than zero, does it have negative value?
-return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
- llvm::Twine(Name) + "." + V->getName() +
- ".negativitycheck");
- };
-
// 1. Was the old Value negative?
- llvm::Value *SrcIsNegative = EmitIsNegativeTest(Src, SrcType, "src");
+ llvm::Value *SrcIsNegative =
+ EmitIsNegativeTestHelper(Src, SrcType, "src", Builder);
// 2. Is the new Value negative?
- llvm::Value *DstIsNegative = EmitIsNegativeTest(Dst, DstType, "dst");
+ llvm::Value *DstIsNegative =
+ EmitIsNegativeTestHelper(Dst, DstType, "dst", Builder);
// 3. Now, was the 'negativity status' preserved during the conversion?
//NOTE: conversion from negative to zero is considered to change the
sign.
//(We want to get 'false' when the conversion changed the sign)
>From 91c943da231de7c10077e431eb79a47d06fe0719 Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:36:04 +0100
Subject: [PATCH 2/3] [clang][UBSan] Add implicit conversion check for
bitfields
This patch implements the implicit truncation and implicit
sign change checks for bitfields.
---
clang/docs/ReleaseNotes.rst | 7 +
clang/docs/UndefinedBehaviorSanitizer.rst | 19 +-
clang/include/clang/Basic/Sanitizers.def | 20 +-
clang/lib/CodeGen/CGExpr.cpp | 43 +++-
clang/lib/CodeGen/CGExprScalar.cpp| 224 +-
clang/lib/CodeGen/CodeGenFunction.h | 12 +
.../test/CodeGen/ubsan-bitfield-conversion.c | 32 +++
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 27 ++-
compiler-rt/lib/ubsan/ubsan_handlers.h| 1 +
9 files changed, 347 insertions(+), 38 deletions(-)
create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b5e6eae9f1bf7d..934be58cd1b9c4 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -143,6 +143,10 @@ Non-comprehensive list of changes in this release
New Compiler Flags
--
+- ``-fsanitize=implicit-bitfield-conversion`` checks implicit truncation and
+ sign change.
+- ``-fsanitize=implicit-integer-conversion`` a group that replaces the previous
+ group ``-fsanitize=implicit-conversion``.
.. _target_os_detail:
@@ -160,6 +164,9 @@ Deprecated Compiler Flags
Modified Compiler Flags
---
+- ``-fsanitize=implicit-conversion`` is now a group for both
+ ``-fsanitize=implicit-integer-conversion`` and
+ ``-fsanitize=implicit-bitfield-conversion``.
Removed Compiler Flags
-
diff --git a/clang/docs/UndefinedBehaviorSanitizer.rst
b/clang/docs/UndefinedBehaviorSanitizer.rst
index b8ad3804f18903..eca82e1b
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
@@ -324,6 +326,19 @@ class ScalarExprEmitter void EmitIntegerSignChangeCheck(Value *Src, QualType SrcType, Value *Dst, QualType DstType, SourceLocation Loc); + /// Emit a check that an [implicit] truncation of a bitfield does not + /// discard any bits. It is not UB, so we use the value after truncation. Zonotora wrote: 👍 https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
@@ -1035,7 +1050,7 @@ EmitIntegerTruncationCheckHelper(Value *Src, QualType SrcType, Value *Dst, } llvm::Value *Check = nullptr; - // 1. Extend the truncated value back to the same width as the Src. + // 1. Convert the Dst back to the same type as Src. Zonotora wrote: You are correct 😃 https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
@@ -1097,6 +1112,27 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
+ const char *Name,
+ CGBuilderTy &Builder) {
+ // NOTE: zero value is considered to be non-negative.
+ // Is this value a signed type?
+ bool VSigned = VType->isSignedIntegerOrEnumerationType();
+ llvm::Type *VTy = V->getType();
+ if (!VSigned) {
+// If the value is unsigned, then it is never negative.
+// FIXME: can we encounter non-scalar VTy here?
Zonotora wrote:
Leftover comment from previous patches, will update.
https://github.com/llvm/llvm-project/pull/75481
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
@@ -1097,6 +1112,27 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
+ const char *Name,
+ CGBuilderTy &Builder) {
+ // NOTE: zero value is considered to be non-negative.
Zonotora wrote:
Leftover comment from previous patches, will update.
https://github.com/llvm/llvm-project/pull/75481
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
@@ -5570,11 +5570,44 @@ LValue CodeGenFunction::EmitBinaryOperatorLValue(const
BinaryOperator *E) {
break;
}
-RValue RV = EmitAnyExpr(E->getRHS());
+llvm::Value *Previous = nullptr;
+RValue RV;
+QualType SrcType = E->getRHS()->getType();
+// If LHS refers to a bitfield we want to retrieve the value before
+// implicit conversion between the bitfield type and the RHS type
+// and evaluate RHS without integer sanitizer checks (if passed)
+if (auto *ICE = RetrieveImplicitCastExprForBitfieldSanitizer(E)) {
Zonotora wrote:
Sounds good!
https://github.com/llvm/llvm-project/pull/75481
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
@@ -122,6 +122,26 @@ Non-comprehensive list of changes in this release New Compiler Flags -- +- ``-fsanitize=implicit-unsigned-bitfield-truncation`` catches implicit + unsigned conversions involving bitfields. +- ``-fsanitize=implicit-signed-bitfield-truncation`` catches implicit + signed conversions involving bitfields. +- ``-fsanitize=implicit-bitfield-sign-change`` catches implicit + conversions involving bitfields that result in a sign change. +- ``-fsanitize=implicit-bitfield-truncation`` a group to include both + ``-fsanitize=implicit-unsigned-bitfield-truncation`` and + ``-fsanitize=implicit-signed-bitfield-truncation``. +- ``-fsanitize=implicit-bitfield-arithmetic-value-change`` a group to + include both ``implicit-signed-bitfield-truncation`` and + ``implicit-bitfield-sign-change``. +- ``-fsanitize=implicit-bitfield-conversion`` a group to include + ``-fsanitize=implicit-unsigned-bitfield-truncation``, + ``-fsanitize=implicit-signed-bitfield-truncation`` and + ``implicit-bitfield-sign-change``. +- ``-fsanitize=implicit-integer-conversion`` a group to include + ``-fsanitize=implicit-unsigned-integer-truncation``, + ``-fsanitize=implicit-signed-integer-truncation`` and + ``implicit-integer-sign-change``. Zonotora wrote: This was my first thought as well, it just seemed intuitive to use the same flags as for integer conversion. I think all checks included in `implicit-conversion` are not UB. Anyway, I will revert to only having `-fsanitize=implicit-bitfield-conversion`! https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
https://github.com/Zonotora updated
https://github.com/llvm/llvm-project/pull/75481
>From b23f9855820196f7ee5741d81f7c408a641a96fd Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:33:21 +0100
Subject: [PATCH 1/3] [clang] Extract negativity check lambda to function
---
clang/lib/CodeGen/CGExprScalar.cpp | 47 --
1 file changed, 25 insertions(+), 22 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index aa805f291d1757..559b1aa7c51023 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -1097,6 +1097,27 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
+ const char *Name,
+ CGBuilderTy &Builder) {
+ // NOTE: zero value is considered to be non-negative.
+ // Is this value a signed type?
+ bool VSigned = VType->isSignedIntegerOrEnumerationType();
+ llvm::Type *VTy = V->getType();
+ if (!VSigned) {
+// If the value is unsigned, then it is never negative.
+// FIXME: can we encounter non-scalar VTy here?
+return llvm::ConstantInt::getFalse(VTy->getContext());
+ }
+ // Get the zero of the same type with which we will be comparing.
+ llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
+ // %V.isnegative = icmp slt %V, 0
+ // I.e is %V *strictly* less than zero, does it have negative value?
+ return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
+llvm::Twine(Name) + "." + V->getName() +
+".negativitycheck");
+};
+
// Should be called within CodeGenFunction::SanitizerScope RAII scope.
// Returns 'i1 false' when the conversion Src -> Dst changed the sign.
static std::pair Value * {
-// Is this value a signed type?
-bool VSigned = VType->isSignedIntegerOrEnumerationType();
-llvm::Type *VTy = V->getType();
-if (!VSigned) {
- // If the value is unsigned, then it is never negative.
- // FIXME: can we encounter non-scalar VTy here?
- return llvm::ConstantInt::getFalse(VTy->getContext());
-}
-// Get the zero of the same type with which we will be comparing.
-llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
-// %V.isnegative = icmp slt %V, 0
-// I.e is %V *strictly* less than zero, does it have negative value?
-return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
- llvm::Twine(Name) + "." + V->getName() +
- ".negativitycheck");
- };
-
// 1. Was the old Value negative?
- llvm::Value *SrcIsNegative = EmitIsNegativeTest(Src, SrcType, "src");
+ llvm::Value *SrcIsNegative =
+ EmitIsNegativeTestHelper(Src, SrcType, "src", Builder);
// 2. Is the new Value negative?
- llvm::Value *DstIsNegative = EmitIsNegativeTest(Dst, DstType, "dst");
+ llvm::Value *DstIsNegative =
+ EmitIsNegativeTestHelper(Dst, DstType, "dst", Builder);
// 3. Now, was the 'negativity status' preserved during the conversion?
//NOTE: conversion from negative to zero is considered to change the
sign.
//(We want to get 'false' when the conversion changed the sign)
>From bc5fe858dc5f7d52b5c032c9b60a385098932f00 Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:36:04 +0100
Subject: [PATCH 2/3] [clang][UBSan] Add implicit conversion check for
bitfields
This patch implements the implicit truncation and implicit
sign change checks for bitfields.
---
clang/docs/ReleaseNotes.rst | 25 ++
clang/docs/UndefinedBehaviorSanitizer.rst | 47 ++-
clang/include/clang/Basic/Sanitizers.def | 34 +-
clang/lib/CodeGen/CGExpr.cpp | 37 +-
clang/lib/CodeGen/CGExprScalar.cpp| 332 +-
clang/lib/CodeGen/CodeGenFunction.h | 14 +
.../test/CodeGen/ubsan-bitfield-conversion.c | 32 ++
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 64 +++-
compiler-rt/lib/ubsan/ubsan_handlers.h| 12 +
compiler-rt/lib/ubsan/ubsan_interface.inc | 2 +
10 files changed, 558 insertions(+), 41 deletions(-)
create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e12a802e2e9ede..eb9aebfd7d25f0 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -122,6 +122,26 @@ Non-comprehensive list of changes in this release
New Compiler Flags
--
+- ``-fsanitize=implicit-unsigned-bitfield-truncation`` catches implicit
+ unsigned conversions involving bitfields.
+- ``-fsanitize=implicit-signed-bitfield-truncation`` catches implicit
+ signed conversions involving bitfields.
+- ``-fsanitize=implicit-bitfield-sign-change`` catches implicit
+ conversio
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
Zonotora wrote:
Hi again, I have now finally gotten time and updated the patch so that the
unnecessary emits I mentioned in the initial commit are avoided. The current
patch introduces a number of new fsanitizer flags to separate integer
conversions from bitfield conversions. E.g.,
- ``-fsanitize=implicit-unsigned-bitfield-truncation``
- ``-fsanitize=implicit-signed-bitfield-truncation``
- ``-fsanitize=implicit-bitfield-sign-change``
- ``-fsanitize=implicit-bitfield-truncation``
- ``-fsanitize=implicit-bitfield-arithmetic-value-change``
- ``-fsanitize=implicit-bitfield-conversion``
- ``-fsanitize=implicit-integer-conversion`` < This used to be
``-fsanitize=implicit-conversion``
``-fsanitize=implicit-conversion`` will now represent
``-fsanitize=implicit-integer-conversion`` and
``-fsanitize=implicit-bitfield-conversion``.
Previously the following:
```c
typedef struct {
unsigned char a:4;
} X;
int main(void) {
X x;
unsigned int a = 272;
x.a = a;
return 0;
}
```
emitted a implict integer conversion error in the assignment of `x.a = a` with
the ``-fsanitize=implicit-integer-conversion``. This is no longer the case as
the assignment involves bitfields. To get the emission error, one would have to
include the ``-fsanitize=implicit-bitfield-conversion`` flag instead.
I have compiled clang with the -fsanitizer flag
``-fsanitize=implicit-bitfield-conversion`` without any problems. What are your
thoughts on this new change? @vitalybuka @AaronBallman @LebedevRI @efriedma-quic
https://github.com/llvm/llvm-project/pull/75481
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
https://github.com/Zonotora edited https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
https://github.com/Zonotora updated
https://github.com/llvm/llvm-project/pull/75481
>From 3b0af99a0e13ed9d6b8c0365a9fa7ea68ec5 Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:33:21 +0100
Subject: [PATCH 1/2] [clang] Extract negativity check lambda to function
---
clang/lib/CodeGen/CGExprScalar.cpp | 47 --
1 file changed, 25 insertions(+), 22 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index df8f71cf1d900..b8caafb00847d 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -1097,6 +1097,27 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
+ const char *Name,
+ CGBuilderTy &Builder) {
+ // NOTE: zero value is considered to be non-negative.
+ // Is this value a signed type?
+ bool VSigned = VType->isSignedIntegerOrEnumerationType();
+ llvm::Type *VTy = V->getType();
+ if (!VSigned) {
+// If the value is unsigned, then it is never negative.
+// FIXME: can we encounter non-scalar VTy here?
+return llvm::ConstantInt::getFalse(VTy->getContext());
+ }
+ // Get the zero of the same type with which we will be comparing.
+ llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
+ // %V.isnegative = icmp slt %V, 0
+ // I.e is %V *strictly* less than zero, does it have negative value?
+ return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
+llvm::Twine(Name) + "." + V->getName() +
+".negativitycheck");
+};
+
// Should be called within CodeGenFunction::SanitizerScope RAII scope.
// Returns 'i1 false' when the conversion Src -> Dst changed the sign.
static std::pair Value * {
-// Is this value a signed type?
-bool VSigned = VType->isSignedIntegerOrEnumerationType();
-llvm::Type *VTy = V->getType();
-if (!VSigned) {
- // If the value is unsigned, then it is never negative.
- // FIXME: can we encounter non-scalar VTy here?
- return llvm::ConstantInt::getFalse(VTy->getContext());
-}
-// Get the zero of the same type with which we will be comparing.
-llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
-// %V.isnegative = icmp slt %V, 0
-// I.e is %V *strictly* less than zero, does it have negative value?
-return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
- llvm::Twine(Name) + "." + V->getName() +
- ".negativitycheck");
- };
-
// 1. Was the old Value negative?
- llvm::Value *SrcIsNegative = EmitIsNegativeTest(Src, SrcType, "src");
+ llvm::Value *SrcIsNegative =
+ EmitIsNegativeTestHelper(Src, SrcType, "src", Builder);
// 2. Is the new Value negative?
- llvm::Value *DstIsNegative = EmitIsNegativeTest(Dst, DstType, "dst");
+ llvm::Value *DstIsNegative =
+ EmitIsNegativeTestHelper(Dst, DstType, "dst", Builder);
// 3. Now, was the 'negativity status' preserved during the conversion?
//NOTE: conversion from negative to zero is considered to change the
sign.
//(We want to get 'false' when the conversion changed the sign)
>From 49e3c6be7ed4e3dcb403b2ea1994dafb6e3d638e Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:36:04 +0100
Subject: [PATCH 2/2] [clang][UBSan] Add implicit conversion check for
bitfields
This patch implements the implicit truncation and implicit
sign change checks for bitfields.
---
clang/docs/ReleaseNotes.rst | 25 ++
clang/docs/UndefinedBehaviorSanitizer.rst | 47 ++-
clang/include/clang/Basic/Sanitizers.def | 32 +-
clang/lib/CodeGen/CGExpr.cpp | 33 +-
clang/lib/CodeGen/CGExprScalar.cpp| 332 +-
clang/lib/CodeGen/CodeGenFunction.h | 14 +
.../test/CodeGen/ubsan-bitfield-conversion.c | 32 ++
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 64 +++-
compiler-rt/lib/ubsan/ubsan_handlers.h| 12 +
compiler-rt/lib/ubsan/ubsan_interface.inc | 2 +
10 files changed, 553 insertions(+), 40 deletions(-)
create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 802c44b6c8608..0f7465f66b53e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -119,6 +119,26 @@ Non-comprehensive list of changes in this release
New Compiler Flags
--
+- ``-fsanitize=implicit-unsigned-bitfield-truncation`` catches implicit
+ unsigned conversions involving bitfields.
+- ``-fsanitize=implicit-signed-bitfield-truncation`` catches implicit
+ signed conversions involving bitfields.
+- ``-fsanitize=implicit-bitfield-sign-change`` catches implicit
+ conversions i
[compiler-rt] [clang] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
Zonotora wrote: > Please add test coverage for compound assignment and increment/decrement. > > This seems like a reasonable extension of the existing integer truncation > checks, but we might want to consider giving it a unique name. Otherwise, > people using integer truncation checks will have trouble upgrading their > compiler. Sure 👍 Fair enough, what name do you suggest? Replace "integer" with "bitfield" as in `implicit-unsigned-bitfield-truncation`, `implicit-signed-bitfield-truncation` and `implicit-bitfield-sign-change`? Or just have a single flag `implicit-bitfield-conversion` for now? https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[compiler-rt] [clang] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
Zonotora wrote: Feedback @vitalybuka @AaronBallman @LebedevRI? :smiley: https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[compiler-rt] [clang] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
Zonotora wrote: > Is is UB? Don't think so? To quote [1]: > Issues caught by these sanitizers are not undefined behavior, but are often > unintentional. where "these sanitizers" refer to `implicit-unsigned-integer-truncation`, `implicit-signed-integer-truncation` and `implicit-integer-sign-change`. [1] https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[compiler-rt] [clang] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
@@ -1094,6 +1114,27 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
Zonotora wrote:
fixed
https://github.com/llvm/llvm-project/pull/75481
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[compiler-rt] [clang] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
https://github.com/Zonotora updated
https://github.com/llvm/llvm-project/pull/75481
>From 8b76571cb7412f3520abf7df5c56cfc987ab7b6e Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:33:21 +0100
Subject: [PATCH 1/2] [clang] Extract negativity check lambda to function
---
clang/lib/CodeGen/CGExprScalar.cpp | 47 --
1 file changed, 25 insertions(+), 22 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index 41ad2ddac30d2d..eb7cd8547d1889 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -1094,6 +1094,27 @@ void ScalarExprEmitter::EmitIntegerTruncationCheck(Value
*Src, QualType SrcType,
{Src, Dst});
}
+static llvm::Value *EmitIsNegativeTestHelper(Value *V, QualType VType,
+ const char *Name,
+ CGBuilderTy &Builder) {
+ // NOTE: zero value is considered to be non-negative.
+ // Is this value a signed type?
+ bool VSigned = VType->isSignedIntegerOrEnumerationType();
+ llvm::Type *VTy = V->getType();
+ if (!VSigned) {
+// If the value is unsigned, then it is never negative.
+// FIXME: can we encounter non-scalar VTy here?
+return llvm::ConstantInt::getFalse(VTy->getContext());
+ }
+ // Get the zero of the same type with which we will be comparing.
+ llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
+ // %V.isnegative = icmp slt %V, 0
+ // I.e is %V *strictly* less than zero, does it have negative value?
+ return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
+llvm::Twine(Name) + "." + V->getName() +
+".negativitycheck");
+};
+
// Should be called within CodeGenFunction::SanitizerScope RAII scope.
// Returns 'i1 false' when the conversion Src -> Dst changed the sign.
static std::pair Value * {
-// Is this value a signed type?
-bool VSigned = VType->isSignedIntegerOrEnumerationType();
-llvm::Type *VTy = V->getType();
-if (!VSigned) {
- // If the value is unsigned, then it is never negative.
- // FIXME: can we encounter non-scalar VTy here?
- return llvm::ConstantInt::getFalse(VTy->getContext());
-}
-// Get the zero of the same type with which we will be comparing.
-llvm::Constant *Zero = llvm::ConstantInt::get(VTy, 0);
-// %V.isnegative = icmp slt %V, 0
-// I.e is %V *strictly* less than zero, does it have negative value?
-return Builder.CreateICmp(llvm::ICmpInst::ICMP_SLT, V, Zero,
- llvm::Twine(Name) + "." + V->getName() +
- ".negativitycheck");
- };
-
// 1. Was the old Value negative?
- llvm::Value *SrcIsNegative = EmitIsNegativeTest(Src, SrcType, "src");
+ llvm::Value *SrcIsNegative =
+ EmitIsNegativeTestHelper(Src, SrcType, "src", Builder);
// 2. Is the new Value negative?
- llvm::Value *DstIsNegative = EmitIsNegativeTest(Dst, DstType, "dst");
+ llvm::Value *DstIsNegative =
+ EmitIsNegativeTestHelper(Dst, DstType, "dst", Builder);
// 3. Now, was the 'negativity status' preserved during the conversion?
//NOTE: conversion from negative to zero is considered to change the
sign.
//(We want to get 'false' when the conversion changed the sign)
>From 73a92d16e06fc7c30bb582b86b9441e7ac560d2d Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Sat, 16 Dec 2023 19:36:04 +0100
Subject: [PATCH 2/2] [clang][UBSan] Add implicit conversion check for
bitfields
This patch implements the implicit truncation and implicit
sign change checks for bitfields. However, right now some
unnecessary emits are generated which ideally would be removed
in the future.
---
clang/lib/CodeGen/CGExprScalar.cpp| 262 +-
.../test/CodeGen/ubsan-bitfield-conversion.c | 27 ++
compiler-rt/lib/ubsan/ubsan_diag.h| 2 +-
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 10 +-
compiler-rt/lib/ubsan/ubsan_handlers.h| 1 +
5 files changed, 293 insertions(+), 9 deletions(-)
create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index eb7cd8547d1889..c93c5d89583d68 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -15,6 +15,7 @@
#include "CGDebugInfo.h"
#include "CGObjCRuntime.h"
#include "CGOpenMPRuntime.h"
+#include "CGRecordLayout.h"
#include "CodeGenFunction.h"
#include "CodeGenModule.h"
#include "ConstantEmitter.h"
@@ -324,6 +325,25 @@ class ScalarExprEmitter
void EmitIntegerSignChangeCheck(Value *Src, QualType SrcType, Value *Dst,
QualType DstType, SourceLocation Loc);
+ /// Emit a check that an [implicit] truncation of a bitfield does not
+ /// discard any bits. It is not UB, so we use the value after truncation.
+ void
[clang] [compiler-rt] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
Zonotora wrote: Hi, @vitalybuka @zygoloid @AaronBallman you might have something to say about this commit https://github.com/llvm/llvm-project/pull/75481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[compiler-rt] [clang] [clang][UBSan] Add implicit conversion check for bitfields (PR #75481)
https://github.com/Zonotora created
https://github.com/llvm/llvm-project/pull/75481
This patch implements the implicit truncation and implicit sign change checks
for bitfields using UBSan. E.g.,
`-fsanitize=implicit-integer-truncation` and
`-fsanitize=implicit-integer-sign-change`.
However, right now some unnecessary emits are generated that ideally will be
removed in the future. Let me explain.
This commit implements the truncation and sign change checks whenever we are
emitting a `EmitStoreThroughBitfieldLValue`. I use the `llvm::Value` updated
through `EmitStoreThroughBitfieldLValue` to check whether the value changed or
not. However, calling `EmitStoreThroughBitfieldLValue` means we have already
evaluated RHS and potentially emitted a truncation or sign check already, which
is the case if we have a bitfield of type `char` and the RHS is of type `int`.
Thus, in this case we may report the truncation or sign change error two times
at runtime under the right circumstances which is not ideal. E.g.,
```c++
#include
#include
typedef struct {
unsigned char a:4;
} X;
int main(void) {
X x;
unsigned int a = 272;
x.a = a;
return 0;
}
```
produces
```bash
main.c:11:11: runtime error: implicit conversion from type 'unsigned int' of
value 272 (32-bit, unsigned) to type 'unsigned char' changed the value to 16
(8-bit, unsigned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior main.c:12:11 in
main.c:11:9: runtime error: implicit conversion from type 'unsigned int' of
value 16 (32-bit, unsigned) to type 'unsigned char' changed the value to 0
(4-bit bitfield, unsigned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior main.c:12:9 in
```
because we truncate the value 272 into a char when evaluating the RHS
(resulting in the value 16) and then truncate it again when assigning it to the
bitfield.
I thought of disabling the relevant sanitizer checks when evaluating the RHS,
but to make it work for all cases I need to extract the previous (before
casting it to char in the above case) `llvm::Value` from `Visit(E->getRHS())`
in order to compare it, which seemed more complicated for a first approach. The
current approach is easier to iterate upon and is more isolated, but again, not
ideal.
I would appreciate feedback so that I can advance this to a mergeable state.
This is my first commit here, so I have probably missed some essentials!
>From 50bcbdf79416baf6a543dc1c3e40bbe7250990d0 Mon Sep 17 00:00:00 2001
From: Zonotora
Date: Thu, 14 Dec 2023 14:38:03 +0100
Subject: [PATCH] [clang][UBSan] Add implicit conversion check for bitfields
This patch implements the implicit truncation and implicit
sign change checks for bitfields. However, right now some
unnecessary emits are generated which ideally would be removed
in the future.
---
clang/lib/CodeGen/CGExprScalar.cpp| 309 --
.../test/CodeGen/ubsan-bitfield-conversion.c | 27 ++
compiler-rt/lib/ubsan/ubsan_diag.h| 2 +-
compiler-rt/lib/ubsan/ubsan_handlers.cpp | 10 +-
compiler-rt/lib/ubsan/ubsan_handlers.h| 1 +
5 files changed, 318 insertions(+), 31 deletions(-)
create mode 100644 clang/test/CodeGen/ubsan-bitfield-conversion.c
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp
b/clang/lib/CodeGen/CGExprScalar.cpp
index 41ad2ddac30d2d..c93c5d89583d68 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -15,6 +15,7 @@
#include "CGDebugInfo.h"
#include "CGObjCRuntime.h"
#include "CGOpenMPRuntime.h"
+#include "CGRecordLayout.h"
#include "CodeGenFunction.h"
#include "CodeGenModule.h"
#include "ConstantEmitter.h"
@@ -324,6 +325,25 @@ class ScalarExprEmitter
void EmitIntegerSignChangeCheck(Value *Src, QualType SrcType, Value *Dst,
QualType DstType, SourceLocation Loc);
+ /// Emit a check that an [implicit] truncation of a bitfield does not
+ /// discard any bits. It is not UB, so we use the value after truncation.
+ void EmitBitfieldTruncationCheck(Value *Src, QualType SrcType, Value *Dst,
+ QualType DstType, const CGBitFieldInfo
&Info,
+ SourceLocation Loc);
+
+ /// Emit a check that an [implicit] conversion of a bitfield does not change
+ /// the sign of the value. It is not UB, so we use the value after
conversion.
+ /// NOTE: Src and Dst may be the exact same value! (point to the same thing)
+ void EmitBitfieldSignChangeCheck(Value *Src, QualType SrcType, Value *Dst,
+ QualType DstType, const CGBitFieldInfo
&Info,
+ SourceLocation Loc);
+
+ /// Emit a check that an [implicit] conversion of a bitfield. It is not UB,
+ /// so we use the value after conversion.
+ void EmitBitfieldConversionCheck(Value *Src, QualType SrcType, Value *Dst,
+ QualType DstType, const CGBitFieldInfo
&Info,
+
