[Clang] Add missing __ob_trap check for sign change - #188340
Merged
Merged
Conversation
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang Author: Justin Stitt (JustinStitt) ChangesAdd a missing OBTrapInvolved check before EmitIntegerSignChangeCheck(). This is considered "missing" as a previous attempt (#185772) to properly add an This backdoor is needed because we want unsigned int __ob_trap big = 4294967295;
(signed int)big; // should trap!Move the CC (because you looked at 185772) Full diff: https://github.com/llvm/llvm-project/pull/188340.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 1fe462c249ca5..749cf7906e9e1 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -1663,6 +1663,20 @@ Value *ScalarExprEmitter::EmitScalarConversion(Value *Src, QualType SrcType,
llvm::Type *DstTy = ConvertType(DstType);
+ // Determine whether an overflow behavior of 'trap' has been specified for
+ // either the destination or the source types. If so, we can elide sanitizer
+ // capability checks as this overflow behavior kind is also capable of
+ // emitting traps without runtime sanitizer support.
+ // Also skip instrumentation if either source or destination has 'wrap'
+ // behavior - the user has explicitly indicated they accept wrapping
+ // semantics. Use non-canonical types to preserve OBT annotations.
+ const auto *DstOBT = NoncanonicalDstType->getAs<OverflowBehaviorType>();
+ const auto *SrcOBT = NoncanonicalSrcType->getAs<OverflowBehaviorType>();
+ bool OBTrapInvolved =
+ (DstOBT && DstOBT->isTrapKind()) || (SrcOBT && SrcOBT->isTrapKind());
+ bool OBWrapInvolved =
+ (DstOBT && DstOBT->isWrapKind()) || (SrcOBT && SrcOBT->isWrapKind());
+
// Cast from half through float if half isn't a native type.
if (SrcType->isHalfType() && !CGF.getContext().getLangOpts().NativeHalfType) {
// Cast to FP using the intrinsic if the half type itself isn't supported.
@@ -1689,9 +1703,10 @@ Value *ScalarExprEmitter::EmitScalarConversion(Value *Src, QualType SrcType,
// Ignore conversions like int -> uint.
if (SrcTy == DstTy) {
- if (Opts.EmitImplicitIntegerSignChangeChecks)
+ if (Opts.EmitImplicitIntegerSignChangeChecks ||
+ (OBTrapInvolved && !OBWrapInvolved))
EmitIntegerSignChangeCheck(Src, NoncanonicalSrcType, Src,
- NoncanonicalDstType, Loc);
+ NoncanonicalDstType, Loc, OBTrapInvolved);
return Src;
}
@@ -1822,20 +1837,6 @@ Value *ScalarExprEmitter::EmitScalarConversion(Value *Src, QualType SrcType,
}
}
- // Determine whether an overflow behavior of 'trap' has been specified for
- // either the destination or the source types. If so, we can elide sanitizer
- // capability checks as this overflow behavior kind is also capable of
- // emitting traps without runtime sanitizer support.
- // Also skip instrumentation if either source or destination has 'wrap'
- // behavior - the user has explicitly indicated they accept wrapping
- // semantics. Use non-canonical types to preserve OBT annotations.
- const auto *DstOBT = NoncanonicalDstType->getAs<OverflowBehaviorType>();
- const auto *SrcOBT = NoncanonicalSrcType->getAs<OverflowBehaviorType>();
- bool OBTrapInvolved =
- (DstOBT && DstOBT->isTrapKind()) || (SrcOBT && SrcOBT->isTrapKind());
- bool OBWrapInvolved =
- (DstOBT && DstOBT->isWrapKind()) || (SrcOBT && SrcOBT->isWrapKind());
-
if ((Opts.EmitImplicitIntegerTruncationChecks || OBTrapInvolved) &&
!OBWrapInvolved)
EmitIntegerTruncationCheck(Src, NoncanonicalSrcType, Res,
diff --git a/clang/test/CodeGen/overflow-behavior-types.c b/clang/test/CodeGen/overflow-behavior-types.c
index cc077c21752d4..4224d288ef89b 100644
--- a/clang/test/CodeGen/overflow-behavior-types.c
+++ b/clang/test/CodeGen/overflow-behavior-types.c
@@ -178,6 +178,7 @@ int explicit_truncation_cast(__ob_trap unsigned long long result) {
// Make sure __ob_trap types warn on sign change with
// -fsanitize=implicit-integer-sign-change or trap otherwise.
// DEFAULT-LABEL: define {{.*}} @unsigned_to_signed_cast
+// NOSAN-LABEL: define {{.*}} @unsigned_to_signed_cast
void unsigned_to_signed_cast(__ob_trap unsigned long long a) {
// DEFAULT: %[[T0:.*]] = load i64, ptr %a.addr
// DEFAULT-NEXT: %[[CONV0:.*]] = trunc i64 %[[T0]] to i8
@@ -194,3 +195,51 @@ void unsigned_to_signed_cast(__ob_trap unsigned long long a) {
// NOSAN-NEXT: br i1 %[[T1]], {{.*}}, label %trap
(signed char)(a);
}
+
+// DEFAULT-LABEL: define {{.*}} @signed_to_unsigned_cast
+// NOSAN-LABEL: define {{.*}} @signed_to_unsigned_cast
+void signed_to_unsigned_cast(__ob_trap signed long long a) {
+ // DEFAULT: %[[T0:.*]] = load i64, ptr %a.addr
+ // DEFAULT-NEXT: %[[CONV0:.*]] = trunc i64 %[[T0]] to i8
+ // DEFAULT: %[[TRUNC0:.*]] = icmp eq i64 {{.*}}, %[[T0]]
+ // DEFAULT-NEXT: br i1 %[[TRUNC0]], {{.*}} %handler.implicit_conversion
+
+ // NOSAN: %[[T0:.*]] = load i64, ptr %a.addr
+ // NOSAN-NEXT: %[[CONV0:.*]] = trunc i64 %[[T0]] to i8
+ // NOSAN: %[[TRUNC0:.*]] = icmp eq i64 {{.*}}, %[[T0]]
+ // NOSAN-NEXT: br i1 %[[TRUNC0]], {{.*}} %trap
+ (unsigned char)(a);
+}
+
+// DEFAULT-LABEL: define {{.*}} @unsigned_to_signed_cast_same_size
+// NOSAN-LABEL: define {{.*}} @unsigned_to_signed_cast_same_size
+void unsigned_to_signed_cast_same_size(__ob_trap unsigned int a) {
+ // DEFAULT: %[[T0:.*]] = load i32, ptr %a.addr
+ // DEFAULT-NEXT: %[[NEG:.*]] = icmp slt i32 %[[T0]], 0
+ // DEFAULT-NEXT: %[[SIGN0:.*]] = icmp eq i1 false, %[[NEG]]
+ // DEFUALT-NEXT: br i1 %[[SIGN0]], {{.*}}, label %handler.implicit_conversion
+
+ // NOSAN: %[[T0:.*]] = load i32, ptr %a.addr
+ // NOSAN-NEXT: %[[NEG:.*]] = icmp slt i32 %[[T0]], 0
+ // NOSAN-NEXT: %[[SIGN0:.*]] = icmp eq i1 false, %[[NEG]]
+ // NOSAN: %[[T1:.*]] = and i1 %[[SIGN0]]
+ // NOSAN-NEXT: br i1 %[[T1]], {{.*}}, label %trap
+ (signed int)(a);
+}
+
+// DEFAULT-LABEL: define {{.*}} @signed_to_unsigned_same_size
+// NOSAN-LABEL: define {{.*}} @signed_to_unsigned_same_size
+void signed_to_unsigned_same_size(__ob_trap signed int a) {
+ // DEFAULT: %[[T0:.*]] = load i32, ptr %a.addr
+ // DEFAULT-NEXT: %[[NEG:.*]] = icmp slt i32 %[[T0]], 0
+ // DEFAULT-NEXT: %[[SIGN0:.*]] = icmp eq i1 %[[NEG]], false
+ // DEFUALT-NEXT: br i1 %[[SIGN0]], {{.*}}, label %handler.implicit_conversion
+
+ // NOSAN: %[[T0:.*]] = load i32, ptr %a.addr
+ // NOSAN-NEXT: %[[NEG:.*]] = icmp slt i32 %[[T0]], 0
+ // NOSAN-NEXT: %[[SIGN0:.*]] = icmp eq i1 %[[NEG]], false
+ // NOSAN: %[[T1:.*]] = and i1 %[[SIGN0]]
+ // NOSAN-NEXT: br i1 %[[T1]], {{.*}}, label %trap
+ (unsigned int)(a);
+}
+
|
Member
|
@llvm/pr-subscribers-clang-codegen Author: Justin Stitt (JustinStitt) ChangesAdd a missing OBTrapInvolved check before EmitIntegerSignChangeCheck(). This is considered "missing" as a previous attempt (#185772) to properly add an This backdoor is needed because we want unsigned int __ob_trap big = 4294967295;
(signed int)big; // should trap!Move the CC (because you looked at 185772) Full diff: https://github.com/llvm/llvm-project/pull/188340.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 1fe462c249ca5..749cf7906e9e1 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -1663,6 +1663,20 @@ Value *ScalarExprEmitter::EmitScalarConversion(Value *Src, QualType SrcType,
llvm::Type *DstTy = ConvertType(DstType);
+ // Determine whether an overflow behavior of 'trap' has been specified for
+ // either the destination or the source types. If so, we can elide sanitizer
+ // capability checks as this overflow behavior kind is also capable of
+ // emitting traps without runtime sanitizer support.
+ // Also skip instrumentation if either source or destination has 'wrap'
+ // behavior - the user has explicitly indicated they accept wrapping
+ // semantics. Use non-canonical types to preserve OBT annotations.
+ const auto *DstOBT = NoncanonicalDstType->getAs<OverflowBehaviorType>();
+ const auto *SrcOBT = NoncanonicalSrcType->getAs<OverflowBehaviorType>();
+ bool OBTrapInvolved =
+ (DstOBT && DstOBT->isTrapKind()) || (SrcOBT && SrcOBT->isTrapKind());
+ bool OBWrapInvolved =
+ (DstOBT && DstOBT->isWrapKind()) || (SrcOBT && SrcOBT->isWrapKind());
+
// Cast from half through float if half isn't a native type.
if (SrcType->isHalfType() && !CGF.getContext().getLangOpts().NativeHalfType) {
// Cast to FP using the intrinsic if the half type itself isn't supported.
@@ -1689,9 +1703,10 @@ Value *ScalarExprEmitter::EmitScalarConversion(Value *Src, QualType SrcType,
// Ignore conversions like int -> uint.
if (SrcTy == DstTy) {
- if (Opts.EmitImplicitIntegerSignChangeChecks)
+ if (Opts.EmitImplicitIntegerSignChangeChecks ||
+ (OBTrapInvolved && !OBWrapInvolved))
EmitIntegerSignChangeCheck(Src, NoncanonicalSrcType, Src,
- NoncanonicalDstType, Loc);
+ NoncanonicalDstType, Loc, OBTrapInvolved);
return Src;
}
@@ -1822,20 +1837,6 @@ Value *ScalarExprEmitter::EmitScalarConversion(Value *Src, QualType SrcType,
}
}
- // Determine whether an overflow behavior of 'trap' has been specified for
- // either the destination or the source types. If so, we can elide sanitizer
- // capability checks as this overflow behavior kind is also capable of
- // emitting traps without runtime sanitizer support.
- // Also skip instrumentation if either source or destination has 'wrap'
- // behavior - the user has explicitly indicated they accept wrapping
- // semantics. Use non-canonical types to preserve OBT annotations.
- const auto *DstOBT = NoncanonicalDstType->getAs<OverflowBehaviorType>();
- const auto *SrcOBT = NoncanonicalSrcType->getAs<OverflowBehaviorType>();
- bool OBTrapInvolved =
- (DstOBT && DstOBT->isTrapKind()) || (SrcOBT && SrcOBT->isTrapKind());
- bool OBWrapInvolved =
- (DstOBT && DstOBT->isWrapKind()) || (SrcOBT && SrcOBT->isWrapKind());
-
if ((Opts.EmitImplicitIntegerTruncationChecks || OBTrapInvolved) &&
!OBWrapInvolved)
EmitIntegerTruncationCheck(Src, NoncanonicalSrcType, Res,
diff --git a/clang/test/CodeGen/overflow-behavior-types.c b/clang/test/CodeGen/overflow-behavior-types.c
index cc077c21752d4..4224d288ef89b 100644
--- a/clang/test/CodeGen/overflow-behavior-types.c
+++ b/clang/test/CodeGen/overflow-behavior-types.c
@@ -178,6 +178,7 @@ int explicit_truncation_cast(__ob_trap unsigned long long result) {
// Make sure __ob_trap types warn on sign change with
// -fsanitize=implicit-integer-sign-change or trap otherwise.
// DEFAULT-LABEL: define {{.*}} @unsigned_to_signed_cast
+// NOSAN-LABEL: define {{.*}} @unsigned_to_signed_cast
void unsigned_to_signed_cast(__ob_trap unsigned long long a) {
// DEFAULT: %[[T0:.*]] = load i64, ptr %a.addr
// DEFAULT-NEXT: %[[CONV0:.*]] = trunc i64 %[[T0]] to i8
@@ -194,3 +195,51 @@ void unsigned_to_signed_cast(__ob_trap unsigned long long a) {
// NOSAN-NEXT: br i1 %[[T1]], {{.*}}, label %trap
(signed char)(a);
}
+
+// DEFAULT-LABEL: define {{.*}} @signed_to_unsigned_cast
+// NOSAN-LABEL: define {{.*}} @signed_to_unsigned_cast
+void signed_to_unsigned_cast(__ob_trap signed long long a) {
+ // DEFAULT: %[[T0:.*]] = load i64, ptr %a.addr
+ // DEFAULT-NEXT: %[[CONV0:.*]] = trunc i64 %[[T0]] to i8
+ // DEFAULT: %[[TRUNC0:.*]] = icmp eq i64 {{.*}}, %[[T0]]
+ // DEFAULT-NEXT: br i1 %[[TRUNC0]], {{.*}} %handler.implicit_conversion
+
+ // NOSAN: %[[T0:.*]] = load i64, ptr %a.addr
+ // NOSAN-NEXT: %[[CONV0:.*]] = trunc i64 %[[T0]] to i8
+ // NOSAN: %[[TRUNC0:.*]] = icmp eq i64 {{.*}}, %[[T0]]
+ // NOSAN-NEXT: br i1 %[[TRUNC0]], {{.*}} %trap
+ (unsigned char)(a);
+}
+
+// DEFAULT-LABEL: define {{.*}} @unsigned_to_signed_cast_same_size
+// NOSAN-LABEL: define {{.*}} @unsigned_to_signed_cast_same_size
+void unsigned_to_signed_cast_same_size(__ob_trap unsigned int a) {
+ // DEFAULT: %[[T0:.*]] = load i32, ptr %a.addr
+ // DEFAULT-NEXT: %[[NEG:.*]] = icmp slt i32 %[[T0]], 0
+ // DEFAULT-NEXT: %[[SIGN0:.*]] = icmp eq i1 false, %[[NEG]]
+ // DEFUALT-NEXT: br i1 %[[SIGN0]], {{.*}}, label %handler.implicit_conversion
+
+ // NOSAN: %[[T0:.*]] = load i32, ptr %a.addr
+ // NOSAN-NEXT: %[[NEG:.*]] = icmp slt i32 %[[T0]], 0
+ // NOSAN-NEXT: %[[SIGN0:.*]] = icmp eq i1 false, %[[NEG]]
+ // NOSAN: %[[T1:.*]] = and i1 %[[SIGN0]]
+ // NOSAN-NEXT: br i1 %[[T1]], {{.*}}, label %trap
+ (signed int)(a);
+}
+
+// DEFAULT-LABEL: define {{.*}} @signed_to_unsigned_same_size
+// NOSAN-LABEL: define {{.*}} @signed_to_unsigned_same_size
+void signed_to_unsigned_same_size(__ob_trap signed int a) {
+ // DEFAULT: %[[T0:.*]] = load i32, ptr %a.addr
+ // DEFAULT-NEXT: %[[NEG:.*]] = icmp slt i32 %[[T0]], 0
+ // DEFAULT-NEXT: %[[SIGN0:.*]] = icmp eq i1 %[[NEG]], false
+ // DEFUALT-NEXT: br i1 %[[SIGN0]], {{.*}}, label %handler.implicit_conversion
+
+ // NOSAN: %[[T0:.*]] = load i32, ptr %a.addr
+ // NOSAN-NEXT: %[[NEG:.*]] = icmp slt i32 %[[T0]], 0
+ // NOSAN-NEXT: %[[SIGN0:.*]] = icmp eq i1 %[[NEG]], false
+ // NOSAN: %[[T1:.*]] = and i1 %[[SIGN0]]
+ // NOSAN-NEXT: br i1 %[[T1]], {{.*}}, label %trap
+ (unsigned int)(a);
+}
+
|
JustinStitt
force-pushed
the
obt-sign-change-same-size
branch
from
March 25, 2026 00:22
ea36abb to
03c0095
Compare
Contributor
|
This is exactly what I needed; thank you! This tracks the type representation change recognition requirements Linux has for these types. |
Contributor
Author
|
ping |
kees
approved these changes
Mar 31, 2026
kees
left a comment
kees
left a comment
Contributor
There was a problem hiding this comment.
Code move with sane logic change. LGTM.
mizvekov
approved these changes
Apr 1, 2026
Comment on lines
+1676
to
+1678
| // semantics. Use non-canonical types to preserve OBT annotations. | ||
| const auto *DstOBT = NoncanonicalDstType->getAs<OverflowBehaviorType>(); | ||
| const auto *SrcOBT = NoncanonicalSrcType->getAs<OverflowBehaviorType>(); |
Contributor
There was a problem hiding this comment.
I realize this is a code move, so preexisting, but:
I am not sure I understand what the comment is trying to say, what information would be lost in the canonical type?
Contributor
Author
There was a problem hiding this comment.
I decided to use these types
QualType NoncanonicalSrcType = SrcType;
QualType NoncanonicalDstType = DstType;as SrcType and DstType undergo various assignments and potential transformations in the function prologue I decided to use the types that I knew were OBTs.
Anyways, I'm sure using SrcType and DstType would have been fine but during development I debug-printed Noncanonical... and they had the right shape :)
Contributor
There was a problem hiding this comment.
I was thinking this could have been a left over from before obt was part of the canonical type.
Anyway, using the canonical type seems to be the right approach here.
JustinStitt
force-pushed
the
obt-sign-change-same-size
branch
from
April 1, 2026 23:58
03c0095 to
b6182bd
Compare
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
Add a missing OBTrapInvolved check before EmitIntegerSignChangeCheck(). This is considered "missing" as a previous attempt [1] to properly add an __ob_trap backdoor missed this particular instance. This backdoor is needed because __ob_trap types are very picky about implicit conversions (including implicit sign change): ```c unsigned int __ob_trap big = 4294967295; (signed int)big; // should trap! ``` [1]: llvm#185772 Signed-off-by: Justin Stitt <justinstitt@google.com>
JustinStitt
force-pushed
the
obt-sign-change-same-size
branch
from
April 2, 2026 16:16
b6182bd to
f33e24d
Compare
zwu-2025
pushed a commit
to zwu-2025/llvm-project
that referenced
this pull request
May 17, 2026
Add a missing OBTrapInvolved check before EmitIntegerSignChangeCheck(). This is considered "missing" as a previous attempt (llvm#185772) to properly add an `__ob_trap` backdoor missed this particular instance. This backdoor is needed because we want `__ob_trap` types to be picky about implicit conversions (including implicit sign change): ```c unsigned int __ob_trap big = 4294967295; (signed int)big; // should trap! ``` Move the `OBTrapInvolved` setup to the top of the function so it can be used in all the places we need it.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Add a missing OBTrapInvolved check before EmitIntegerSignChangeCheck().
This is considered "missing" as a previous attempt (#185772) to properly add an
__ob_trapbackdoor missed this particular instance.This backdoor is needed because we want
__ob_traptypes to be picky about implicit conversions (including implicit sign change):Move the
OBTrapInvolvedsetup to the top of the function so it can be used in all the places we need it.CC (because you looked at 185772)
@mizvekov @vitalybuka @kees