kees wrote:
With PR #82432 landed, this PR is redundant. Thanks for changing the option
name! Closing...
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://github.com/kees closed https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
MaskRay wrote:
> > I believe we can move forward by reusing
> > `-fsanitize=signed-integer-overflow`, which adds least complexity to Clang
> > and is very reasonable.
>
> I see a few problems with changing `-fsanitize=signed-integer-overflow`:
>
> 1. Clang no longer matches GCC's SIO
https://github.com/JustinStitt edited
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -193,7 +193,14 @@ Available checks are:
signed division overflow (``INT_MIN/-1``), but not checks for
lossy implicit conversions performed before the computation
(see ``-fsanitize=implicit-conversion``). Both of these two issues are
- handled by
https://github.com/JustinStitt edited
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/JustinStitt edited
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -193,7 +193,14 @@ Available checks are:
signed division overflow (``INT_MIN/-1``), but not checks for
lossy implicit conversions performed before the computation
(see ``-fsanitize=implicit-conversion``). Both of these two issues are
- handled by
https://github.com/JustinStitt edited
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -193,7 +193,14 @@ Available checks are:
signed division overflow (``INT_MIN/-1``), but not checks for
lossy implicit conversions performed before the computation
(see ``-fsanitize=implicit-conversion``). Both of these two issues are
- handled by
@@ -193,7 +193,14 @@ Available checks are:
signed division overflow (``INT_MIN/-1``), but not checks for
lossy implicit conversions performed before the computation
(see ``-fsanitize=implicit-conversion``). Both of these two issues are
- handled by
@@ -193,7 +193,14 @@ Available checks are:
signed division overflow (``INT_MIN/-1``), but not checks for
lossy implicit conversions performed before the computation
(see ``-fsanitize=implicit-conversion``). Both of these two issues are
- handled by
@@ -193,7 +193,14 @@ Available checks are:
signed division overflow (``INT_MIN/-1``), but not checks for
lossy implicit conversions performed before the computation
(see ``-fsanitize=implicit-conversion``). Both of these two issues are
- handled by
JustinStitt wrote:
> I believe we can move forward by reusing
> `-fsanitize=signed-integer-overflow`, which adds least complexity to Clang
> and is very reasonable.
I see a few problems with changing `-fsanitize=signed-integer-overflow`:
1) Clang no longer matches GCC's SIO functionality
2)
https://github.com/MaskRay requested changes to this pull request.
.
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
MaskRay wrote:
> GCC folks have not answered. Adding -wrap keeps the behavior for -overflow
> the same between GCC and Clang. Can we please move this forward and land it
> as is? We can trivially change this in the future if we need to.
I believe we can move forward by reusing
kees wrote:
GCC folks have not answered. Adding -wrap keeps the behavior for -overflow the
same between GCC and Clang. Can we please move this forward and land it as is?
We can trivially change this in the future if we need to.
https://github.com/llvm/llvm-project/pull/80089
JustinStitt wrote:
My original idea was to get the SIO sanitizer working with `-fwrapv`, the issue
[here](https://github.com/KSPP/linux/issues/26) even suggests it as a viable
option. However, after seeing literal checks like:
```cpp
case LangOptions::SOB_Undefined:
if
kees wrote:
> Sure -fwrapv makes wraparound defined, but it doesn't prevent us from making
> -fsanitize=signed-integer-overflow useful. "-fwrapv => no
> signed-integer-overflow" is not a solid argument.
>
> I think we can try making -fsanitize=signed-integer-overflow effective even
> when
kees wrote:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102317
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
MaskRay wrote:
This is a UI discussion about how command line options should behave.
Some folks prefer simpler rules while some prefer smart rules (guessing what
the user intends).
A
[-fwrapv](https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html#index-fwrapv)
user may either:
* rely on
kees wrote:
> > > > > Why not just enforce -fsanitize=signed-integer-overflow with -fwrapv?
> > > > > I suspect it's just overlook, and not intentional behavior.
> > > >
> > > >
> > > > +1
> > > > We should consider this direction
> > >
> > >
> > > The UB-vs-non-UB seemed to be a really
rjmccall wrote:
> > > > Why not just enforce -fsanitize=signed-integer-overflow with -fwrapv? I
> > > > suspect it's just overlook, and not intentional behavior.
> > >
> > >
> > > +1
> > > We should consider this direction
> >
> >
> > The UB-vs-non-UB seemed to be a really specific goal in
vitalybuka wrote:
> `signed-integer-overflow`
Or maybe I am missing the bigger picture? Is there a plan fix
signed-integer-overflow for -fwrapv as well?
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
vitalybuka wrote:
> > > Why not just enforce -fsanitize=signed-integer-overflow with -fwrapv? I
> > > suspect it's just overlook, and not intentional behavior.
> >
> >
> > +1
> > We should consider this direction
>
> The UB-vs-non-UB seemed to be a really specific goal in the existing code.
kees wrote:
> > Why not just enforce -fsanitize=signed-integer-overflow with -fwrapv? I
> > suspect it's just overlook, and not intentional behavior.
>
> +1
>
> We should consider this direction
The UB-vs-non-UB seemed to be a really specific goal in the existing code. i.e.
that the
MaskRay wrote:
> Why not just enforce -fsanitize=signed-integer-overflow with -fwrapv? I
> suspect it's just overlook, and not intentional behavior.
+1
We should consider this direction
https://github.com/llvm/llvm-project/pull/80089
___
vitalybuka wrote:
Why not just enforce -fsanitize=signed-integer-overflow with -fwrapv? I suspect
it's just overlook, and not intentional behavior.
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
https://github.com/rjmccall commented:
Yeah, LGTM.
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
AaronBallman wrote:
> I think this patch is good to go. Thanks for working on it!
>
> cc @AaronBallman @efriedma-quic @rjmccall @nikic in case there's any last
> minute concerns. I'm happy to sign off on this, but consider giving folks
> some time to speak up before landing this.
No concerns
JustinStitt wrote:
> I think this patch is good to go. Thanks for working on it!
Thanks for the review Nick! BTW, I forgot to update the `ReleaseNotes.rst`,
I've done so now (hopefully not voiding your previous review)
https://github.com/llvm/llvm-project/pull/80089
https://github.com/JustinStitt updated
https://github.com/llvm/llvm-project/pull/80089
>From 7774e4036ac1de7fdf5fe4c6b3208b492853ffc5 Mon Sep 17 00:00:00 2001
From: Justin Stitt
Date: Tue, 23 Jan 2024 23:28:42 +
Subject: [PATCH 01/11] add signed-integer-wrap sanitizer
---
https://github.com/JustinStitt edited
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/nickdesaulniers approved this pull request.
I think this patch is good to go. Thanks for working on it!
cc @AaronBallman @efriedma-quic @rjmccall @nikic in case there's any last
minute concerns. I'm happy to sign off on this, but consider giving folks some
time to speak up
JustinStitt wrote:
> UBSan is documented clang/docs/UndefinedBehaviorSanitizer.rst
> https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
>
> Please make sure to update the list [of available
> checks](https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#available-checks).
>
>
https://github.com/JustinStitt updated
https://github.com/llvm/llvm-project/pull/80089
>From 7774e4036ac1de7fdf5fe4c6b3208b492853ffc5 Mon Sep 17 00:00:00 2001
From: Justin Stitt
Date: Tue, 23 Jan 2024 23:28:42 +
Subject: [PATCH 01/10] add signed-integer-wrap sanitizer
---
https://github.com/JustinStitt updated
https://github.com/llvm/llvm-project/pull/80089
>From 7774e4036ac1de7fdf5fe4c6b3208b492853ffc5 Mon Sep 17 00:00:00 2001
From: Justin Stitt
Date: Tue, 23 Jan 2024 23:28:42 +
Subject: [PATCH 1/9] add signed-integer-wrap sanitizer
---
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
JustinStitt wrote:
Gotcha, I did just that in
[5497e8b](https://github.com/llvm/llvm-project/pull/80089/commits/5497e8bc6849bf64c1158ff16b4aa04fd9141920).
Thanks Nick!
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s
-fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CHECKSIW
+
+// Check that
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s
-fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CHECKSIW
+
+// Check that
@@ -3554,12 +3572,20 @@ Value
*ScalarExprEmitter::EmitOverflowCheckedBinOp(const BinOpInfo ) {
const std::string *handlerName =
().OverflowHandler;
if (handlerName->empty()) {
-// If the signed-integer-overflow sanitizer is enabled, emit a call to its
-//
https://github.com/JustinStitt updated
https://github.com/llvm/llvm-project/pull/80089
>From 7774e4036ac1de7fdf5fe4c6b3208b492853ffc5 Mon Sep 17 00:00:00 2001
From: Justin Stitt
Date: Tue, 23 Jan 2024 23:28:42 +
Subject: [PATCH 1/8] add signed-integer-wrap sanitizer
---
https://github.com/JustinStitt updated
https://github.com/llvm/llvm-project/pull/80089
>From 7774e4036ac1de7fdf5fe4c6b3208b492853ffc5 Mon Sep 17 00:00:00 2001
From: Justin Stitt
Date: Tue, 23 Jan 2024 23:28:42 +
Subject: [PATCH 1/6] add signed-integer-wrap sanitizer
---
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s
-fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CHECKSIW
+
+// Check that
kees wrote:
> > and likely in production kernels. :)
>
> I doubt you meant running in production.
I very much do -- I expect to use this like we use the array-bounds sanitizer,
which is on in production in at least Ubuntu and Android. It may be a long road
to getting sane coverage without
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
nickdesaulniers wrote:
> I must be doing something wrong because it suggests I remove all my Check:
> ...'s and replace them with Check: {{.*}}
Huh, indeed. I can repro.
https://github.com/nickdesaulniers edited
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/nickdesaulniers commented:
PATCH mostly LGTM
UBSan is documented clang/docs/UndefinedBehaviorSanitizer.rst
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
Please make sure to update the list [of available
@@ -3554,12 +3572,20 @@ Value
*ScalarExprEmitter::EmitOverflowCheckedBinOp(const BinOpInfo ) {
const std::string *handlerName =
().OverflowHandler;
if (handlerName->empty()) {
-// If the signed-integer-overflow sanitizer is enabled, emit a call to its
-//
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s
-fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CHECKSIW
+
+// Check that
kees wrote:
I've tested this with the Linux kernel, and it is working as expected. It is
ready and waiting for this option to gain back a bunch of sanitizer coverage
for CIs and likely in production kernels. :)
https://github.com/llvm/llvm-project/pull/80089
JustinStitt wrote:
ping!
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
kees wrote:
> Hey, does anyone know why the CI bot (buildkite) fails my builds for a
> trailing whitespace in a file I did not touch?
>
> It says there's some trailing whitespace in some documentation files that my
> PR doesn't touch (unless I'm misinterpreting its output):
>
https://github.com/JustinStitt edited
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s
-fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CHECKSIW
+
+// Check that
https://github.com/JustinStitt deleted
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
JustinStitt wrote:
I believe @tschuett was saying that to be confident we are instrumenting the
arithmetic (and the correct operands therein) we should check the overflow
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s
-fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CHECKSIW
+
+// Check that
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s
-fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CHECKSIW
+
+// Check that
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
nickdesaulniers wrote:
We don't have to use `update_cc_test_checks`; it's nice when we can and when
the test has many CHECK lines, but I suspect most of the CHECK lines in
JustinStitt wrote:
Hey, does anyone know why the CI bot (buildkite) fails my builds for a trailing
whitespace in a file I did not touch?
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s
-fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CHECK
nickdesaulniers wrote:
If there
https://github.com/JustinStitt edited
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s
-fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CHECK
JustinStitt wrote:
> `CHECK` is
https://github.com/JustinStitt edited
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s
-fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CHECK
nickdesaulniers wrote:
`CHECK`
https://github.com/JustinStitt edited
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
JustinStitt wrote:
I must be doing something wrong because it suggest I remove all my `Check:
...`'s and replcae them with `Check: {{.*}}`
@@ -0,0 +1,77 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
nickdesaulniers wrote:
Have you seen `llvm/utils/update_cc_test_checks.py` before? Honestly, ALL new
tests to clang (or llvm) involving codegen should strive to use that
https://github.com/JustinStitt edited
https://github.com/llvm/llvm-project/pull/80089
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -0,0 +1,66 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s
-fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CHECK
+
+// Check that
https://github.com/JustinStitt updated
https://github.com/llvm/llvm-project/pull/80089
>From 68805d7871230033be43c1d87dfcd2aa2b668589 Mon Sep 17 00:00:00 2001
From: Justin Stitt
Date: Tue, 23 Jan 2024 23:28:42 +
Subject: [PATCH 1/5] add signed-integer-wrap sanitizer
---
https://github.com/JustinStitt updated
https://github.com/llvm/llvm-project/pull/80089
>From 68805d7871230033be43c1d87dfcd2aa2b668589 Mon Sep 17 00:00:00 2001
From: Justin Stitt
Date: Tue, 23 Jan 2024 23:28:42 +
Subject: [PATCH 1/4] add signed-integer-wrap sanitizer
---
nickdesaulniers wrote:
> I tried running clang-format but the diff was huge and beyond just my changes.
I usually do `git clang-format HEAD~N` where N is the number of commits of mine
(N is optional when N == 1).
https://github.com/llvm/llvm-project/pull/80089
@@ -0,0 +1,66 @@
+// Check that -fsanitize=signed-integer-wrap instruments with -fwrapv
+// RUN: %clang_cc1 -fwrapv -triple x86_64-apple-darwin -emit-llvm -o - %s
-fsanitize=signed-integer-wrap | FileCheck %s --check-prefix=CHECK
+
+// Check that
DavidSpickett wrote:
> I tried running clang-format but the diff was huge and beyond just my changes.
https://clang.llvm.org/docs/ClangFormat.html#git-integration tells you how to
narrow it down to just your changes. Once that's setup you can use the command
from the comment the bot left.
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff 8b38970811086b09752a5909d0c17de4d0cd04c3
a94835f633164a33aaf413b92ecf9ec96701add8 --
llvmbot wrote:
@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-compiler-rt-sanitizer
Author: Justin Stitt (JustinStitt)
Changes
**Reasoning**
Clang has a `signed-integer-overflow` sanitizer to catch arithmetic overflow;
however, most* of its instrumentation [fails to
github-actions[bot] wrote:
Thank you for submitting a Pull Request (PR) to the LLVM Project!
This PR will be automatically labeled and the relevant teams will be
notified.
If you wish to, you can add reviewers by using the "Reviewers" section on this
page.
If this is not working for you, it
https://github.com/JustinStitt created
https://github.com/llvm/llvm-project/pull/80089
**Reasoning**
Clang has a `signed-integer-overflow` sanitizer to catch arithmetic overflow;
however, most* of its instrumentation [fails to
apply](https://godbolt.org/z/ee41rE8o6) when `-fwrapv` is enabled.
80 matches
Mail list logo
