Skip to content

[Clang] Add missing __ob_trap check for sign change #188340

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JustinStitt merged 1 commit into from
Apr 2, 2026
Merged

[Clang] Add missing __ob_trap check for sign change #188340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 29 additions & 24 deletions clang/lib/CodeGen/CGExprScalar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1355,14 +1355,18 @@ void ScalarExprEmitter::EmitIntegerSignChangeCheck(Value *Src, QualType SrcType,
return;
}
// Does an SSCL have an entry for the DstType under its respective sanitizer
// section?
if (DstSigned && CGF.getContext().isTypeIgnoredBySanitizer(
SanitizerKind::ImplicitSignedIntegerTruncation, DstType))
return;
if (!DstSigned &&
CGF.getContext().isTypeIgnoredBySanitizer(
SanitizerKind::ImplicitUnsignedIntegerTruncation, DstType))
return;
// section? Don't check this if an __ob_trap type is involved as it has
// priority to emit checks regardless of sanitizer case lists.
if (!OBTrapInvolved) {
if (DstSigned &&
CGF.getContext().isTypeIgnoredBySanitizer(
SanitizerKind::ImplicitSignedIntegerTruncation, DstType))
return;
if (!DstSigned &&
CGF.getContext().isTypeIgnoredBySanitizer(
SanitizerKind::ImplicitUnsignedIntegerTruncation, DstType))
return;
}
// That's it. We can't rule out any more cases with the data we have.

auto CheckHandler = SanitizerHandler::ImplicitConversion;
Expand Down Expand Up @@ -1670,6 +1674,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>();
Comment on lines +1683 to +1685

@mizvekov mizvekov Apr 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@JustinStitt JustinStitt Apr 1, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

@mizvekov mizvekov Apr 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
Expand All @@ -1696,9 +1714,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;
}
Expand Down Expand Up @@ -1829,20 +1848,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 && !Opts.PatternExcluded)
EmitIntegerTruncationCheck(Src, NoncanonicalSrcType, Res,
Expand Down
17 changes: 17 additions & 0 deletions clang/test/CodeGen/overflow-behavior-types-scl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@
// RUN: -fexperimental-overflow-behavior-types -fsanitize=implicit-unsigned-integer-truncation,implicit-signed-integer-truncation \
// RUN: -emit-llvm -o - | FileCheck %s --check-prefix=TRUNC

// RUN: %clang_cc1 -triple x86_64-linux-gnu %t/test.c -fsanitize-ignorelist=%t/sign-change.scl \
// RUN: -fexperimental-overflow-behavior-types -fsanitize=implicit-integer-sign-change \
// RUN: -emit-llvm -o - | FileCheck %s --check-prefix=SIGNCHG

//--- sio.scl
[signed-integer-overflow]
# ignore signed-integer-overflow instrumentation across all types
Expand All @@ -25,6 +29,10 @@ type:*
[{implicit-unsigned-integer-truncation,implicit-signed-integer-truncation}]
type:*

//--- sign-change.scl
[implicit-integer-sign-change]
type:*

//--- test.c
#define __wrap __attribute__((overflow_behavior("wrap")))
#define __no_trap __attribute__((overflow_behavior("trap")))
Expand Down Expand Up @@ -62,3 +70,12 @@ void bar(int value) {
// TRUNC-NEXT: br i1 %[[TCHECK]], {{.*}}%handler.implicit_conversion
unsigned char __ob_trap a = value;
}

// SIGNCHG-LABEL: define {{.*}} @obt_priority_over_scl_for_sign_change_sanitizer
void obt_priority_over_scl_for_sign_change_sanitizer(unsigned int __ob_trap a) {
// SIGNCHG: %[[T0:.*]] = load i32, ptr %a.addr
// SIGNCHG-NEXT: %[[NEG0:.*]] = icmp slt i32 %[[T0]], 0
// SIGNCHG-NEXT: %[[SIGN0:.*]] = icmp eq i1 false, %[[NEG0]]
// SIGNCHG-NEXT: br i1 %[[SIGN0]], {{.*}} %handler.implicit_conversion
(signed int)a;
}
59 changes: 54 additions & 5 deletions clang/test/CodeGen/overflow-behavior-types.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,9 +175,18 @@ int explicit_truncation_cast(__ob_trap unsigned long long result) {
return (int)result;
}

// EXCL-LABEL: define {{.*}} @pattern_exclusion_priority_over_obt
void pattern_exclusion_priority_over_obt(unsigned char __ob_trap count) {
// EXCL: while.cond
// EXCL: br i1 %tobool, label %while.body
// EXCL: br label %while.cond
while (count--) {}
}

// 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
Expand All @@ -195,9 +204,49 @@ void unsigned_to_signed_cast(__ob_trap unsigned long long a) {
(signed char)(a);
}

// EXCL-LABEL: define {{.*}} @pattern_exclusion_priority_over_obt
void pattern_exclusion_priority_over_obt(unsigned char __ob_trap count) {
// EXCL: br label %while.cond
// EXCL-NOT: %truncheck
while (count--) {}
// 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);
}