[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin
artem updated this revision to Diff 550013.
artem marked 5 inline comments as done.
artem edited the summary of this revision.
artem added a comment.
Fixed review comments
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156821/new/
https://reviews.llvm.org/D156821
Files:
clang/docs/ReleaseNotes.rst
clang/lib/CodeGen/CGBuiltin.cpp
clang/test/CodeGen/abs-overflow.c
compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
Index: compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
@@ -0,0 +1,30 @@
+// REQUIRES: arch=x86_64
+//
+// RUN: %clangxx -fsanitize=signed-integer-overflow -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
+//
+// RUN: %clangxx -fsanitize=signed-integer-overflow -fno-sanitize-recover=signed-integer-overflow -w %s -O3 -o %t.abort
+// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
+
+#include
+#include
+
+int main() {
+ // ABORT: abs.cpp:[[@LINE+3]]:17: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself
+ // RECOVER: abs.cpp:[[@LINE+2]]:17: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself
+ // RECOVER: abs.cpp:[[@LINE+2]]:7: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself
+ __builtin_abs(INT_MIN);
+ abs(INT_MIN);
+
+ // RECOVER: abs.cpp:[[@LINE+2]]:18: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'long'; cast to an unsigned type to negate this value to itself
+ // RECOVER: abs.cpp:[[@LINE+2]]:8: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'long'; cast to an unsigned type to negate this value to itself
+ __builtin_labs(LONG_MIN);
+ labs(LONG_MIN);
+
+ // RECOVER: abs.cpp:[[@LINE+2]]:19: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'long long'; cast to an unsigned type to negate this value to itself
+ // RECOVER: abs.cpp:[[@LINE+2]]:9: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'long long'; cast to an unsigned type to negate this value to itself
+ __builtin_llabs(LLONG_MIN);
+ llabs(LLONG_MIN);
+
+ return 0;
+}
Index: clang/test/CodeGen/abs-overflow.c
===
--- /dev/null
+++ clang/test/CodeGen/abs-overflow.c
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm %s -o - -fwrapv -fsanitize=signed-integer-overflow | FileCheck %s --check-prefix=WRAPV
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm %s -o - -ftrapv | FileCheck %s --check-prefixes=BOTH-TRAP,TRAPV
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm %s -o - -fsanitize=signed-integer-overflow | FileCheck %s --check-prefixes=BOTH-TRAP,CATCH_UB
+// COM: TODO: Support -ftrapv-handler.
+
+extern int abs(int x);
+
+int absi(int x) {
+// WRAPV: [[NEG:%.*]] = sub i32 0, [[X:%.*]]
+// WRAPV: [[CMP:%.*]] = icmp slt i32 [[X]], 0
+// WRAPV: [[SEL:%.*]] = select i1 [[CMP]], i32 [[NEG]], i32 [[X]]
+//
+// BOTH-TRAP: [[NEG:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 0, i32 [[X:%.*]])
+// BOTH-TRAP: [[NEGV:%.*]] = extractvalue { i32, i1 } [[NEG]], 0
+// BOTH-TRAP: [[OFL:%.*]] = extractvalue { i32, i1 } [[NEG]], 1
+// BOTH-TRAP: [[NOFL:%.*]] = xor i1 [[OFL]], true
+// BOTH-TRAP: br i1 [[NOFL]], label %[[CONT:.*]], label %[[TRAP:[a-zA-Z_.]*]]
+// BOTH-TRAP: [[TRAP]]:
+// TRAPV-NEXT:llvm.ubsantrap
+// CATCH_UB: @__ubsan_handle_negate_overflow
+// BOTH-TRAP-NEXT:unreachable
+// BOTH-TRAP: [[CONT]]:
+// BOTH-TRAP-NEXT:[[ABSCOND:%.*]] = icmp slt i32 [[X]], 0
+// BOTH-TRAP-NEXT:select i1 [[ABSCOND]], i32 [[NEGV]], i32 [[X]]
+ return abs(x);
+}
+
+int babsi(int x) {
+// WRAPV: [[NEG:%.*]] = sub i32 0, [[X:%.*]]
+// WRAPV: [[CMP:%.*]] = icmp slt i32 [[X]], 0
+// WRAPV: [[SEL:%.*]] = select i1 [[CMP]], i32 [[NEG]], i32 [[X]]
+//
+// BOTH-TRAP: [[NEG:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 0, i32 [[X:%.*]])
+// BOTH-TRAP: [[NEGV:%.*]] = extractvalue { i32, i1 } [[NEG]], 0
+// BOTH-TRAP: [[OFL:%.*]] = extractvalue { i32, i1 } [[NEG]], 1
+// BOTH-TRAP: [[NOFL:%.*]] = xor i1 [[OFL]], true
+// BOTH-TRAP: br i1 [[NOFL]], label %[[CONT:.*]], label %[[TRAP:[a-zA-Z_.]*]]
+// BOTH-TRAP: [[TRAP]]:
+// TRAPV-NEXT:llvm.ubsantrap
+// CATCH_UB: @__ubsan_handle_negate_overflow
+// BOTH-TRAP-NEXT:unreachable
+// BOTH-TRAP: [[CONT]]:
+// BOTH-TRAP-NEXT:[[ABSCOND:%.*]] = icmp slt i32 [[X]], 0
+// BOTH-TRAP-NEXT:select i1 [[ABSCOND]], i32 [[NEGV]], i32 [[X]]
+ return __builtin_abs(x);
+}
Index: clang/lib/CodeGen/CGBuiltin.cpp
==
[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin
artem added a comment. Thanks for the review. I do not have commit rights, could you please push it? Artem Labazov <123321art...@gmail.com> CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156821/new/ https://reviews.llvm.org/D156821 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin
artem added a comment. Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156821/new/ https://reviews.llvm.org/D156821 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin
artem updated this revision to Diff 551656.
artem marked 2 inline comments as done.
artem added a comment.
Fixed the comments. I do not have commit rights
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156821/new/
https://reviews.llvm.org/D156821
Files:
clang/docs/ReleaseNotes.rst
clang/lib/CodeGen/CGBuiltin.cpp
clang/test/CodeGen/abs-overflow.c
compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
Index: compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
@@ -0,0 +1,28 @@
+// RUN: %clangxx -fsanitize=signed-integer-overflow -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
+//
+// RUN: %clangxx -fsanitize=signed-integer-overflow -fno-sanitize-recover=signed-integer-overflow -w %s -O3 -o %t.abort
+// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
+
+#include
+#include
+
+int main() {
+ // ABORT: abs.cpp:[[@LINE+3]]:17: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself
+ // RECOVER: abs.cpp:[[@LINE+2]]:17: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself
+ // RECOVER: abs.cpp:[[@LINE+2]]:7: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself
+ __builtin_abs(INT_MIN);
+ abs(INT_MIN);
+
+ // RECOVER: abs.cpp:[[@LINE+2]]:18: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'long'; cast to an unsigned type to negate this value to itself
+ // RECOVER: abs.cpp:[[@LINE+2]]:8: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'long'; cast to an unsigned type to negate this value to itself
+ __builtin_labs(LONG_MIN);
+ labs(LONG_MIN);
+
+ // RECOVER: abs.cpp:[[@LINE+2]]:19: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'long long'; cast to an unsigned type to negate this value to itself
+ // RECOVER: abs.cpp:[[@LINE+2]]:9: runtime error: negation of -{{[0-9]+}} cannot be represented in type 'long long'; cast to an unsigned type to negate this value to itself
+ __builtin_llabs(LLONG_MIN);
+ llabs(LLONG_MIN);
+
+ return 0;
+}
Index: clang/test/CodeGen/abs-overflow.c
===
--- /dev/null
+++ clang/test/CodeGen/abs-overflow.c
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm %s -o - -fwrapv -fsanitize=signed-integer-overflow | FileCheck %s --check-prefix=WRAPV
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm %s -o - -ftrapv | FileCheck %s --check-prefixes=BOTH-TRAP,TRAPV
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm %s -o - -fsanitize=signed-integer-overflow | FileCheck %s --check-prefixes=BOTH-TRAP,CATCH_UB
+// COM: TODO: Support -ftrapv-handler.
+
+extern int abs(int x);
+
+int absi(int x) {
+// WRAPV: [[NEG:%.*]] = sub i32 0, [[X:%.*]]
+// WRAPV: [[CMP:%.*]] = icmp slt i32 [[X]], 0
+// WRAPV: [[SEL:%.*]] = select i1 [[CMP]], i32 [[NEG]], i32 [[X]]
+//
+// BOTH-TRAP: [[NEG:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 0, i32 [[X:%.*]])
+// BOTH-TRAP: [[NEGV:%.*]] = extractvalue { i32, i1 } [[NEG]], 0
+// BOTH-TRAP: [[OFL:%.*]] = extractvalue { i32, i1 } [[NEG]], 1
+// BOTH-TRAP: [[NOFL:%.*]] = xor i1 [[OFL]], true
+// BOTH-TRAP: br i1 [[NOFL]], label %[[CONT:.*]], label %[[TRAP:[a-zA-Z_.]*]]
+// BOTH-TRAP: [[TRAP]]:
+// TRAPV-NEXT:llvm.ubsantrap
+// CATCH_UB: @__ubsan_handle_negate_overflow
+// BOTH-TRAP-NEXT:unreachable
+// BOTH-TRAP: [[CONT]]:
+// BOTH-TRAP-NEXT:[[ABSCOND:%.*]] = icmp slt i32 [[X]], 0
+// BOTH-TRAP-NEXT:select i1 [[ABSCOND]], i32 [[NEGV]], i32 [[X]]
+ return abs(x);
+}
+
+int babsi(int x) {
+// WRAPV: [[NEG:%.*]] = sub i32 0, [[X:%.*]]
+// WRAPV: [[CMP:%.*]] = icmp slt i32 [[X]], 0
+// WRAPV: [[SEL:%.*]] = select i1 [[CMP]], i32 [[NEG]], i32 [[X]]
+//
+// BOTH-TRAP: [[NEG:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 0, i32 [[X:%.*]])
+// BOTH-TRAP: [[NEGV:%.*]] = extractvalue { i32, i1 } [[NEG]], 0
+// BOTH-TRAP: [[OFL:%.*]] = extractvalue { i32, i1 } [[NEG]], 1
+// BOTH-TRAP: [[NOFL:%.*]] = xor i1 [[OFL]], true
+// BOTH-TRAP: br i1 [[NOFL]], label %[[CONT:.*]], label %[[TRAP:[a-zA-Z_.]*]]
+// BOTH-TRAP: [[TRAP]]:
+// TRAPV-NEXT:llvm.ubsantrap
+// CATCH_UB: @__ubsan_handle_negate_overflow
+// BOTH-TRAP-NEXT:unreachable
+// BOTH-TRAP: [[CONT]]:
+// BOTH-TRAP-NEXT:[[ABSCOND:%.*]] = icmp slt i32 [[X]], 0
+// BOTH-TRAP-NEXT:select i1 [[ABSCOND]], i32 [[NEGV]], i32 [[X]]
+ return __builtin_abs(x);
+}
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang
[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin
artem added inline comments.
Comment at: clang/test/ubsan/TestCases/Misc/abs.cpp:1
+// REQUIRES: arch=x86_64
+//
MaskRay wrote:
> Did you mean compiler-rt/ instead of clang/?
>
> `// REQUIRES: arch=x86_64` is legacy style.
>
> New style uses `// REQUIRES: target={{x86_64.*}}`
>
> Actually, this test should be generic and we should remove REQUIRES.
I think that Aaron has moved the file when committed to the main branch.
Anyway, moved.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156821/new/
https://reviews.llvm.org/D156821
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin
artem updated this revision to Diff 551966.
artem marked an inline comment as done.
artem added a comment.
Rebased and fixed legacy FileCheck syntax. Gentle reminder that I do not have
commit rights.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156821/new/
https://reviews.llvm.org/D156821
Files:
clang/docs/ReleaseNotes.rst
clang/lib/CodeGen/CGBuiltin.cpp
clang/test/CodeGen/abs-overflow.c
compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
Index: compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
@@ -0,0 +1,28 @@
+// RUN: %clangxx -fsanitize=signed-integer-overflow -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
+//
+// RUN: %clangxx -fsanitize=signed-integer-overflow -fno-sanitize-recover=signed-integer-overflow -w %s -O3 -o %t.abort
+// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
+
+#include
+#include
+
+int main() {
+ // ABORT: abs.cpp:[[#@LINE+3]]:17: runtime error: negation of -[[#]] cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself
+ // RECOVER: abs.cpp:[[#@LINE+2]]:17: runtime error: negation of -[[#]] cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself
+ // RECOVER: abs.cpp:[[#@LINE+2]]:7: runtime error: negation of -[[#]] cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself
+ __builtin_abs(INT_MIN);
+ abs(INT_MIN);
+
+ // RECOVER: abs.cpp:[[#@LINE+2]]:18: runtime error: negation of -[[#]] cannot be represented in type 'long'; cast to an unsigned type to negate this value to itself
+ // RECOVER: abs.cpp:[[#@LINE+2]]:8: runtime error: negation of -[[#]] cannot be represented in type 'long'; cast to an unsigned type to negate this value to itself
+ __builtin_labs(LONG_MIN);
+ labs(LONG_MIN);
+
+ // RECOVER: abs.cpp:[[#@LINE+2]]:19: runtime error: negation of -[[#]] cannot be represented in type 'long long'; cast to an unsigned type to negate this value to itself
+ // RECOVER: abs.cpp:[[#@LINE+2]]:9: runtime error: negation of -[[#]] cannot be represented in type 'long long'; cast to an unsigned type to negate this value to itself
+ __builtin_llabs(LLONG_MIN);
+ llabs(LLONG_MIN);
+
+ return 0;
+}
Index: clang/test/CodeGen/abs-overflow.c
===
--- /dev/null
+++ clang/test/CodeGen/abs-overflow.c
@@ -0,0 +1,46 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm %s -o - -fwrapv -fsanitize=signed-integer-overflow | FileCheck %s --check-prefix=WRAPV
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm %s -o - -ftrapv | FileCheck %s --check-prefixes=BOTH-TRAP,TRAPV
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm %s -o - -fsanitize=signed-integer-overflow | FileCheck %s --check-prefixes=BOTH-TRAP,CATCH_UB
+// COM: TODO: Support -ftrapv-handler.
+
+extern int abs(int x);
+
+int absi(int x) {
+// WRAPV: [[NEG:%.*]] = sub i32 0, [[X:%.*]]
+// WRAPV: [[CMP:%.*]] = icmp slt i32 [[X]], 0
+// WRAPV: [[SEL:%.*]] = select i1 [[CMP]], i32 [[NEG]], i32 [[X]]
+//
+// BOTH-TRAP: [[NEG:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 0, i32 [[X:%.*]])
+// BOTH-TRAP: [[NEGV:%.*]] = extractvalue { i32, i1 } [[NEG]], 0
+// BOTH-TRAP: [[OFL:%.*]] = extractvalue { i32, i1 } [[NEG]], 1
+// BOTH-TRAP: [[NOFL:%.*]] = xor i1 [[OFL]], true
+// BOTH-TRAP: br i1 [[NOFL]], label %[[CONT:.*]], label %[[TRAP:[a-zA-Z_.]*]]
+// BOTH-TRAP: [[TRAP]]:
+// TRAPV-NEXT:llvm.ubsantrap
+// CATCH_UB: @__ubsan_handle_negate_overflow
+// BOTH-TRAP-NEXT:unreachable
+// BOTH-TRAP: [[CONT]]:
+// BOTH-TRAP-NEXT:[[ABSCOND:%.*]] = icmp slt i32 [[X]], 0
+// BOTH-TRAP-NEXT:select i1 [[ABSCOND]], i32 [[NEGV]], i32 [[X]]
+ return abs(x);
+}
+
+int babsi(int x) {
+// WRAPV: [[NEG:%.*]] = sub i32 0, [[X:%.*]]
+// WRAPV: [[CMP:%.*]] = icmp slt i32 [[X]], 0
+// WRAPV: [[SEL:%.*]] = select i1 [[CMP]], i32 [[NEG]], i32 [[X]]
+//
+// BOTH-TRAP: [[NEG:%.*]] = call { i32, i1 } @llvm.ssub.with.overflow.i32(i32 0, i32 [[X:%.*]])
+// BOTH-TRAP: [[NEGV:%.*]] = extractvalue { i32, i1 } [[NEG]], 0
+// BOTH-TRAP: [[OFL:%.*]] = extractvalue { i32, i1 } [[NEG]], 1
+// BOTH-TRAP: [[NOFL:%.*]] = xor i1 [[OFL]], true
+// BOTH-TRAP: br i1 [[NOFL]], label %[[CONT:.*]], label %[[TRAP:[a-zA-Z_.]*]]
+// BOTH-TRAP: [[TRAP]]:
+// TRAPV-NEXT:llvm.ubsantrap
+// CATCH_UB: @__ubsan_handle_negate_overflow
+// BOTH-TRAP-NEXT:unreachable
+// BOTH-TRAP: [[CONT]]:
+// BOTH-TRAP-NEXT:[[ABSCOND:%.*]] = icmp slt i32 [[X]], 0
+// BOTH-TRAP-NEXT:select i1 [[ABSCOND]], i32 [[NEGV]], i32 [[X]]
+ return __builtin_abs(x);
+}
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin
artem added inline comments.
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1787
+if (!VCI->isMinSignedValue()) {
+ return EmitAbs(CGF, ArgValue, true);
+}
MaskRay wrote:
> MaskRay wrote:
> > nit: we delete braces in this cascading case
> >
> > https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
> Thanks. I think the coding standard omits the outer braces as well.
Coding standard says "Use braces on the outer `if`"
Comment at: compiler-rt/test/ubsan/TestCases/Misc/abs.cpp:11
+int main() {
+ // ABORT: abs.cpp:[[@LINE+3]]:17: runtime error: negation of -{{[0-9]+}}
cannot be represented in type 'int'; cast to an unsigned type to negate this
value to itself
+ // RECOVER: abs.cpp:[[@LINE+2]]:17: runtime error: negation of -{{[0-9]+}}
cannot be represented in type 'int'; cast to an unsigned type to negate this
value to itself
MaskRay wrote:
> FYI: `[[@LINE+3]]` is deprecated lit syntax
> https://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-pseudo-numeric-variables.
> New code should use `[[#@LINE+3]]`
>
> `{{[0-9]+}}` can be simplified as `[[#]]`
Thanks! Fixed
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156821/new/
https://reviews.llvm.org/D156821
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin
artem created this revision.
artem added a reviewer: craig.topper.
Herald added subscribers: Enna1, kbarton, nemanjai.
Herald added a project: All.
artem requested review of this revision.
Herald added projects: clang, Sanitizers.
Herald added subscribers: Sanitizers, cfe-commits.
Currenly both Clang and GCC support the following set of flags that control
code gen of signed overflow:
- -fwrapv: overflow is defined as in two-complement
- -ftrapv: overflow traps
- -fsanitize=signed-integer-overflow: if undefined (no -fwrapv), then overflow
behaviour is controlled by UBSan runtime, overrides -ftrapv
Howerver, clang ignores these flags for __builtin_abs(int) and its higher-width
versions, so passing minimum integer value always causes poison.
The same holds for *abs(), which are not handled in frontend at all but folded
to llvm.abs.* intrinsics during InstCombinePass. The intrinsics are not
instrumented by UBSan, so the functions need special handling as well.
This patch does a few things:
- Handle *abs() in CGBuiltin the same way as __builtin_*abs()
- Clang now emits llvm.abs.* intrinsic where possible
- -fsanitize=builtin,signed-integer-overflow now properly instruments abs()
with UBSan
- -fwrapv and -ftrapv handling for abs() is made consistent with GCC
Fixes #45129 and #45794
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D156821
Files:
clang/lib/CodeGen/CGBuiltin.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-macros.c
clang/test/CodeGen/abs-overflow.c
clang/test/CodeGen/builtin-abs.c
clang/test/CodeGenCXX/builtins.cpp
compiler-rt/lib/ubsan/ubsan_handlers.cpp
compiler-rt/lib/ubsan/ubsan_handlers.h
Index: compiler-rt/lib/ubsan/ubsan_handlers.h
===
--- compiler-rt/lib/ubsan/ubsan_handlers.h
+++ compiler-rt/lib/ubsan/ubsan_handlers.h
@@ -158,6 +158,7 @@
enum BuiltinCheckKind : unsigned char {
BCK_CTZPassedZero,
BCK_CLZPassedZero,
+ BCK_AbsPassedBadVal,
};
struct InvalidBuiltinData {
Index: compiler-rt/lib/ubsan/ubsan_handlers.cpp
===
--- compiler-rt/lib/ubsan/ubsan_handlers.cpp
+++ compiler-rt/lib/ubsan/ubsan_handlers.cpp
@@ -626,9 +626,13 @@
ScopedReport R(Opts, Loc, ET);
- Diag(Loc, DL_Error, ET,
- "passing zero to %0, which is not a valid argument")
-<< ((Data->Kind == BCK_CTZPassedZero) ? "ctz()" : "clz()");
+ if (Data->Kind == BCK_AbsPassedBadVal) {
+Diag(Loc, DL_Error, ET,
+ "passing minimal signed integer to abs(), which results in overflow");
+ } else {
+Diag(Loc, DL_Error, ET, "passing zero to %0, which is not a valid argument")
+<< ((Data->Kind == BCK_CTZPassedZero) ? "ctz()" : "clz()");
+ }
}
void __ubsan::__ubsan_handle_invalid_builtin(InvalidBuiltinData *Data) {
Index: clang/test/CodeGenCXX/builtins.cpp
===
--- clang/test/CodeGenCXX/builtins.cpp
+++ clang/test/CodeGenCXX/builtins.cpp
@@ -53,7 +53,8 @@
extern "C" int __builtin_abs(int); // #3
int x = __builtin_abs(-2);
-// CHECK: store i32 2, ptr @x, align 4
+// CHECK: [[X:%.*]] = call i32 @llvm.abs.i32(i32 -2, i1 true)
+// CHECK: store i32 [[X]], ptr @x, align 4
long y = __builtin_abs(-2l);
// CHECK: [[Y:%.+]] = call noundef i64 @_Z13__builtin_absl(i64 noundef -2)
Index: clang/test/CodeGen/builtin-abs.c
===
--- clang/test/CodeGen/builtin-abs.c
+++ clang/test/CodeGen/builtin-abs.c
@@ -2,27 +2,21 @@
int absi(int x) {
// CHECK-LABEL: @absi(
-// CHECK: [[NEG:%.*]] = sub nsw i32 0, [[X:%.*]]
-// CHECK: [[CMP:%.*]] = icmp slt i32 [[X]], 0
-// CHECK: [[SEL:%.*]] = select i1 [[CMP]], i32 [[NEG]], i32 [[X]]
+// CHECK: [[ABS:%.*]] = call i32 @llvm.abs.i32(i32 [[X:%.*]], i1 true)
//
return __builtin_abs(x);
}
long absl(long x) {
// CHECK-LABEL: @absl(
-// CHECK: [[NEG:%.*]] = sub nsw i64 0, [[X:%.*]]
-// CHECK: [[CMP:%.*]] = icmp slt i64 [[X]], 0
-// CHECK: [[SEL:%.*]] = select i1 [[CMP]], i64 [[NEG]], i64 [[X]]
+// CHECK: [[ABS:%.*]] = call i64 @llvm.abs.i64(i64 [[X:%.*]], i1 true)
//
return __builtin_labs(x);
}
long long absll(long long x) {
// CHECK-LABEL: @absll(
-// CHECK: [[NEG:%.*]] = sub nsw i64 0, [[X:%.*]]
-// CHECK: [[CMP:%.*]] = icmp slt i64 [[X]], 0
-// CHECK: [[SEL:%.*]] = select i1 [[CMP]], i64 [[NEG]], i64 [[X]]
+// CHECK: [[ABS:%.*]] = call i64 @llvm.abs.i64(i64 [[X:%.*]], i1 true)
//
return __builtin_llabs(x);
}
Index: clang/test/CodeGen/abs-overflow.c
===
--- /dev/null
+++ clang/test/CodeGen/abs-overflow.c
@@ -0,0 +1,38 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 2
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm %s -o - -fwrapv -fsanitize=builtin
[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin
artem updated this revision to Diff 546261.
artem added a comment.
Fixed comments and failing test. Added a new compiler-rt test for ubsan
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156821/new/
https://reviews.llvm.org/D156821
Files:
clang/lib/CodeGen/CGBuiltin.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-macros.c
clang/test/CodeGen/abs-overflow.c
clang/test/CodeGen/builtin-abs.c
clang/test/CodeGenCXX/builtins.cpp
compiler-rt/lib/ubsan/ubsan_handlers.cpp
compiler-rt/lib/ubsan/ubsan_handlers.h
compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
Index: compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
@@ -0,0 +1,26 @@
+// REQUIRES: arch=x86_64
+//
+// RUN: %clangxx -fsanitize=builtin -fno-sanitize-recover=signed-integer-overflow -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
+// RUN: %clangxx -fsanitize=signed-integer-overflow -fno-sanitize-recover=builtin -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
+//
+// RUN: %clangxx -fsanitize=builtin -fsanitize=signed-integer-overflow -fno-sanitize-recover=builtin -w %s -O3 -o %t.abort
+// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
+// RUN: %clangxx -fsanitize=builtin -fsanitize=signed-integer-overflow -fno-sanitize-recover=signed-integer-overflow -w %s -O3 -o %t.abort
+// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
+
+#include
+
+int main() {
+ // ABORT: abs.cpp:[[@LINE+2]]:17: runtime error: passing a minimal signed integer to abs(), which results in overflow
+ // RECOVER: abs.cpp:[[@LINE+1]]:17: runtime error: passing a minimal signed integer to abs(), which results in overflow
+ __builtin_abs(INT_MIN);
+
+ // RECOVER: abs.cpp:[[@LINE+1]]:18: runtime error: passing a minimal signed integer to abs(), which results in overflow
+ __builtin_labs(LONG_MIN);
+
+ // RECOVER: abs.cpp:[[@LINE+1]]:19: runtime error: passing a minimal signed integer to abs(), which results in overflow
+ __builtin_llabs(LLONG_MIN);
+ return 0;
+}
Index: compiler-rt/lib/ubsan/ubsan_handlers.h
===
--- compiler-rt/lib/ubsan/ubsan_handlers.h
+++ compiler-rt/lib/ubsan/ubsan_handlers.h
@@ -158,6 +158,7 @@
enum BuiltinCheckKind : unsigned char {
BCK_CTZPassedZero,
BCK_CLZPassedZero,
+ BCK_AbsPassedBadVal,
};
struct InvalidBuiltinData {
Index: compiler-rt/lib/ubsan/ubsan_handlers.cpp
===
--- compiler-rt/lib/ubsan/ubsan_handlers.cpp
+++ compiler-rt/lib/ubsan/ubsan_handlers.cpp
@@ -626,9 +626,13 @@
ScopedReport R(Opts, Loc, ET);
- Diag(Loc, DL_Error, ET,
- "passing zero to %0, which is not a valid argument")
-<< ((Data->Kind == BCK_CTZPassedZero) ? "ctz()" : "clz()");
+ if (Data->Kind == BCK_AbsPassedBadVal) {
+Diag(Loc, DL_Error, ET,
+ "passing minimal signed integer to abs(), which results in overflow");
+ } else {
+Diag(Loc, DL_Error, ET, "passing zero to %0, which is not a valid argument")
+<< ((Data->Kind == BCK_CTZPassedZero) ? "ctz()" : "clz()");
+ }
}
void __ubsan::__ubsan_handle_invalid_builtin(InvalidBuiltinData *Data) {
Index: clang/test/CodeGenCXX/builtins.cpp
===
--- clang/test/CodeGenCXX/builtins.cpp
+++ clang/test/CodeGenCXX/builtins.cpp
@@ -53,7 +53,8 @@
extern "C" int __builtin_abs(int); // #3
int x = __builtin_abs(-2);
-// CHECK: store i32 2, ptr @x, align 4
+// CHECK: [[X:%.*]] = call i32 @llvm.abs.i32(i32 -2, i1 true)
+// CHECK: store i32 [[X]], ptr @x, align 4
long y = __builtin_abs(-2l);
// CHECK: [[Y:%.+]] = call noundef i64 @_Z13__builtin_absl(i64 noundef -2)
Index: clang/test/CodeGen/builtin-abs.c
===
--- clang/test/CodeGen/builtin-abs.c
+++ clang/test/CodeGen/builtin-abs.c
@@ -2,27 +2,21 @@
int absi(int x) {
// CHECK-LABEL: @absi(
-// CHECK: [[NEG:%.*]] = sub nsw i32 0, [[X:%.*]]
-// CHECK: [[CMP:%.*]] = icmp slt i32 [[X]], 0
-// CHECK: [[SEL:%.*]] = select i1 [[CMP]], i32 [[NEG]], i32 [[X]]
+// CHECK: [[ABS:%.*]] = call i32 @llvm.abs.i32(i32 [[X:%.*]], i1 true)
//
return __builtin_abs(x);
}
long absl(long x) {
// CHECK-LABEL: @absl(
-// CHECK: [[NEG:%.*]] = sub nsw i64 0, [[X:%.*]]
-// CHECK: [[CMP:%.*]] = icmp slt i64 [[X]], 0
-// CHECK: [[SEL:%.*]] = select i1 [[CMP]], i64 [[NEG]], i64 [[X]]
+// CHECK: [[ABS:%.*]] = call i64 @llvm.abs.i64(i64 [[X:%.*]], i1 true)
//
return __builtin_labs(x);
}
long long absll(long long x) {
// CHECK-LABEL: @absll(
-// CHECK: [[NEG:%.*]] = sub nsw i64 0, [[X:%.*]]
-// CHECK: [[CMP:%.*]] = icmp slt i64 [[X]], 0
-// CHECK: [[SEL:%.*]] = select i1 [[CMP]], i64 [[NEG]], i64 [[
[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin
artem updated this revision to Diff 546269.
artem added a comment.
typo fix
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156821/new/
https://reviews.llvm.org/D156821
Files:
clang/lib/CodeGen/CGBuiltin.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/test/CodeGen/PowerPC/builtins-ppc-xlcompat-macros.c
clang/test/CodeGen/abs-overflow.c
clang/test/CodeGen/builtin-abs.c
clang/test/CodeGenCXX/builtins.cpp
compiler-rt/lib/ubsan/ubsan_handlers.cpp
compiler-rt/lib/ubsan/ubsan_handlers.h
compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
Index: compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
===
--- /dev/null
+++ compiler-rt/test/ubsan/TestCases/Misc/abs.cpp
@@ -0,0 +1,26 @@
+// REQUIRES: arch=x86_64
+//
+// RUN: %clangxx -fsanitize=builtin -fno-sanitize-recover=signed-integer-overflow -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
+// RUN: %clangxx -fsanitize=signed-integer-overflow -fno-sanitize-recover=builtin -w %s -O3 -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=RECOVER
+//
+// RUN: %clangxx -fsanitize=builtin -fsanitize=signed-integer-overflow -fno-sanitize-recover=builtin -w %s -O3 -o %t.abort
+// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
+// RUN: %clangxx -fsanitize=builtin -fsanitize=signed-integer-overflow -fno-sanitize-recover=signed-integer-overflow -w %s -O3 -o %t.abort
+// RUN: not %run %t.abort 2>&1 | FileCheck %s --check-prefix=ABORT
+
+#include
+
+int main() {
+ // ABORT: abs.cpp:[[@LINE+2]]:17: runtime error: passing minimal signed integer to abs(), which overflows
+ // RECOVER: abs.cpp:[[@LINE+1]]:17: runtime error: passing minimal signed integer to abs(), which overflows
+ __builtin_abs(INT_MIN);
+
+ // RECOVER: abs.cpp:[[@LINE+1]]:18: runtime error: passing minimal signed integer to abs(), which overflows
+ __builtin_labs(LONG_MIN);
+
+ // RECOVER: abs.cpp:[[@LINE+1]]:19: runtime error: passing minimal signed integer to abs(), which overflows
+ __builtin_llabs(LLONG_MIN);
+ return 0;
+}
Index: compiler-rt/lib/ubsan/ubsan_handlers.h
===
--- compiler-rt/lib/ubsan/ubsan_handlers.h
+++ compiler-rt/lib/ubsan/ubsan_handlers.h
@@ -158,6 +158,7 @@
enum BuiltinCheckKind : unsigned char {
BCK_CTZPassedZero,
BCK_CLZPassedZero,
+ BCK_AbsPassedBadVal,
};
struct InvalidBuiltinData {
Index: compiler-rt/lib/ubsan/ubsan_handlers.cpp
===
--- compiler-rt/lib/ubsan/ubsan_handlers.cpp
+++ compiler-rt/lib/ubsan/ubsan_handlers.cpp
@@ -626,9 +626,13 @@
ScopedReport R(Opts, Loc, ET);
- Diag(Loc, DL_Error, ET,
- "passing zero to %0, which is not a valid argument")
-<< ((Data->Kind == BCK_CTZPassedZero) ? "ctz()" : "clz()");
+ if (Data->Kind == BCK_AbsPassedBadVal) {
+Diag(Loc, DL_Error, ET,
+ "passing minimal signed integer to abs(), which overflows");
+ } else {
+Diag(Loc, DL_Error, ET, "passing zero to %0, which is not a valid argument")
+<< ((Data->Kind == BCK_CTZPassedZero) ? "ctz()" : "clz()");
+ }
}
void __ubsan::__ubsan_handle_invalid_builtin(InvalidBuiltinData *Data) {
Index: clang/test/CodeGenCXX/builtins.cpp
===
--- clang/test/CodeGenCXX/builtins.cpp
+++ clang/test/CodeGenCXX/builtins.cpp
@@ -53,7 +53,8 @@
extern "C" int __builtin_abs(int); // #3
int x = __builtin_abs(-2);
-// CHECK: store i32 2, ptr @x, align 4
+// CHECK: [[X:%.*]] = call i32 @llvm.abs.i32(i32 -2, i1 true)
+// CHECK: store i32 [[X]], ptr @x, align 4
long y = __builtin_abs(-2l);
// CHECK: [[Y:%.+]] = call noundef i64 @_Z13__builtin_absl(i64 noundef -2)
Index: clang/test/CodeGen/builtin-abs.c
===
--- clang/test/CodeGen/builtin-abs.c
+++ clang/test/CodeGen/builtin-abs.c
@@ -2,27 +2,21 @@
int absi(int x) {
// CHECK-LABEL: @absi(
-// CHECK: [[NEG:%.*]] = sub nsw i32 0, [[X:%.*]]
-// CHECK: [[CMP:%.*]] = icmp slt i32 [[X]], 0
-// CHECK: [[SEL:%.*]] = select i1 [[CMP]], i32 [[NEG]], i32 [[X]]
+// CHECK: [[ABS:%.*]] = call i32 @llvm.abs.i32(i32 [[X:%.*]], i1 true)
//
return __builtin_abs(x);
}
long absl(long x) {
// CHECK-LABEL: @absl(
-// CHECK: [[NEG:%.*]] = sub nsw i64 0, [[X:%.*]]
-// CHECK: [[CMP:%.*]] = icmp slt i64 [[X]], 0
-// CHECK: [[SEL:%.*]] = select i1 [[CMP]], i64 [[NEG]], i64 [[X]]
+// CHECK: [[ABS:%.*]] = call i64 @llvm.abs.i64(i64 [[X:%.*]], i1 true)
//
return __builtin_labs(x);
}
long long absll(long long x) {
// CHECK-LABEL: @absll(
-// CHECK: [[NEG:%.*]] = sub nsw i64 0, [[X:%.*]]
-// CHECK: [[CMP:%.*]] = icmp slt i64 [[X]], 0
-// CHECK: [[SEL:%.*]] = select i1 [[CMP]], i64 [[NEG]], i64 [[X]]
+// CHECK: [[ABS:%.*]] = call i64 @llvm.abs.i64(i64 [[X:%.*]], i1 true)
//
return __builtin_llabs(x);
}
Index:
[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin
artem added a comment. Ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156821/new/ https://reviews.llvm.org/D156821 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin
artem added a comment. Thanks for the feedback! I will resolve the problems in the next revision after some clarifications. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2697 +bool SanitizeBuiltin = SanOpts.has(SanitizerKind::Builtin); +bool SanitizeOverflow = SanOpts.has(SanitizerKind::SignedIntegerOverflow); + efriedma wrote: > Putting the behavior under both "builtin" and "signed-integer-overflow" feels > a little weird; is there any precedent for that? The problem is, we are instrumenting a builtin function, so on the one hand it is reasonable to be controlled by `-fsanitize=builtin`. On the other hand, GCC treats abs() as an arithmetic operation, so it is being instrumented by `-fsanitize=signed-integer-overflow` (`abs(INT_MIN)` causes a negation overflow). I have decided to enable instrumentation under any of the flags, but I am not sure whether it is the right choice. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2702 +case LangOptions::SOB_Defined: + Result = Builder.CreateBinaryIntrinsic( + Intrinsic::abs, EmitScalarExpr(E->getArg(0)), Builder.getFalse(), efriedma wrote: > Can we land the change to directly generate calls to llvm.abs() in the > default case separately? This might end up impacting generated code for a > variety of workloads, and I'd prefer to have a more clear bisect point. I used llvm.abs() for code simplicity, since middle-end combines the instructions to it anyways. I think this part can be dropped entirely because the intrinsic is not the best possible option either (the compiler emits conditional move and fails to elide extra sign checks if the argument is known to be non-negative). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156821/new/ https://reviews.llvm.org/D156821 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
