JustinStitt wrote:
@nico
> The test added here was tweaked a bit and then deleted in
> [07a8cba](https://github.com/llvm/llvm-project/commit/07a8cbaf8dc16bebf6e875173d20299d9cc47cc5)
>
> What gives?
>
> Instead of deleting tests, we should revert the PR that adds them and then
> reland the P
nico wrote:
The test added here was tweaked a bit and then deleted in
https://github.com/llvm/llvm-project/commit/07a8cbaf8dc16bebf6e875173d20299d9cc47cc5
What gives?
Instead of deleting tests, we should revert the PR that adds them and then
reland the PR with working tests.
https://github.c
vitalybuka wrote:
> > Can we split `-fsanitize=unsigned-integer-overflow` into
> > `-fsanitize=unsigned-integer-overflow-patternA,unsigned-integer-overflow-patternB,unsigned-integer-overflow-patternC...`
> > ?
> > Then it's quite intuitive to disable them with `no-sanitize`.
>
> Yikes, no way.
kees wrote:
> Can we split `-fsanitize=unsigned-integer-overflow` into
> `-fsanitize=unsigned-integer-overflow-patternA,unsigned-integer-overflow-patternB,unsigned-integer-overflow-patternC...`
> ?
>
> Then it's quite intuitive to disable them with `no-sanitize`.
Yikes, no way. The pattern ex
vitalybuka wrote:
> > Before reland, please include me into review I'd like to understand why
> > `-fsanitize-pattern-exclusion=all` is better than something like
> > `-fno-sanitize=overflow-pattern-all`
>
> The latter doesn't make sense to me. `no-sanitize` takes a list of sanitizers
> to co
JustinStitt wrote:
> Sorry, but I am not sure why this didn't show up my
> https://github.com/llvm/llvm-project/pulls?q=is%3Aopen+is%3Apr+review-requested%3A%40me+sort%3Aupdated-desc
> I wanted to review this patch.
Did I do something wrong with my PR or fork settings? I am not sure why you
w
kees wrote:
> Before reland, please include me into review I'd like to understand why
> `-fsanitize-pattern-exclusion=all` is better than something like
> `-fno-sanitize=overflow-pattern-all`
The latter doesn't make sense to me. `no-sanitize` takes a list of sanitizers
to completely disable.
bwendling wrote:
@vitalybuka Are they still broken? Which ones?
https://github.com/llvm/llvm-project/pull/100272
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
vitalybuka wrote:
Before reland, please include me into review
I'd like to understand why
`-fsanitize-pattern-exclusion=all` is better than something like
`-fno-sanitize=overflow-pattern-all`
https://github.com/llvm/llvm-project/pull/100272
___
cfe-co
vitalybuka wrote:
We should revert this patch. It breaks mupltiple bots, introducing Msan reports.
Also on a first look I don't like `-fsanitize-pattern-exclusion=` at all, very
inconsistent.
I assumed we agreed to splitting sanitizers into smaller sets?
I would like to know why we need to intr
bwendling wrote:
I removed it temporarily.
https://github.com/llvm/llvm-project/pull/100272
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
dyung wrote:
> Hopefully
> [6e2d9df](https://github.com/llvm/llvm-project/commit/6e2d9df02502e16659e4a9397260baf9df224f17)
> fixes the buildbots. I think we should using update_cc_test_checks.py to
> generate these testcases though.
The test overflow-idiom-exclusion-fp.c appears to have been
bwendling wrote:
Hopefully 6e2d9df02502 fixes the buildbots. I think we should using
update_cc_test_checks.py to generate these testcases though.
https://github.com/llvm/llvm-project/pull/100272
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
JustinStitt wrote:
@bwendling we want to use `-triple` not `-target`, can you cherry pick
3ef83ad323dfbe99028c774c3716706671ba7b8b
I also reordered the tests and added labels so the tests are actually checking
the right stuff -- debugging was really difficult but will be easier now.
https://g
dyung wrote:
@bwendling The follow-up commit you made to the tests added here
(7275919cd5fc89c42a52168c9f4411b4e5421c95) seems to have caused the test to
start failing on at least a few bots.
- https://lab.llvm.org/buildbot/#/builders/154/builds/2752
- https://lab.llvm.org/buildbot/#/builders/
llvm-ci wrote:
LLVM Buildbot has detected a new failure on builder `clang-ve-ninja` running on
`hpce-ve-main` while building `clang` at step 4 "annotate".
Full details are available at:
https://lab.llvm.org/buildbot/#/builders/12/builds/3786
Here is the relevant piece of the build log for the
https://github.com/bwendling closed
https://github.com/llvm/llvm-project/pull/100272
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/JustinStitt updated
https://github.com/llvm/llvm-project/pull/100272
>From 154d3505ab13275086b3dffed67bcdcac52f79a3 Mon Sep 17 00:00:00 2001
From: Justin Stitt
Date: Tue, 23 Jul 2024 20:21:49 +
Subject: [PATCH 01/13] implement idiom exclusions
Add flag `-fno-sanitize-ove
https://github.com/efriedma-quic approved this pull request.
LGTM
The other thing you could do is modify ScalarExprEmitter::EmitCompare... if it
sees an overflow pattern, it passes that down as an argument to the visit of
the add. But I guess given the current structure of the code, that gets
bwendling wrote:
> @efriedma-quic
>
> > I think serialization is missing for the new bit on BinaryOperator.
>
> How do I add this?
>
> > I'm not sure why we're storing it in the first place, though; it's queried
> > in exactly one place, so there isn't really any benefit to precomputing it.
>
https://github.com/JustinStitt updated
https://github.com/llvm/llvm-project/pull/100272
>From 154d3505ab13275086b3dffed67bcdcac52f79a3 Mon Sep 17 00:00:00 2001
From: Justin Stitt
Date: Tue, 23 Jul 2024 20:21:49 +
Subject: [PATCH 01/12] implement idiom exclusions
Add flag `-fno-sanitize-ove
JustinStitt wrote:
> I think serialization is missing for the new bit on BinaryOperator.
How do I add this?
> I'm not sure why we're storing it in the first place, though; it's queried in
> exactly one place, so there isn't really any benefit to precomputing it.
It's queried when we check
https://github.com/efriedma-quic commented:
I think serialization is missing for the new bit on BinaryOperator. I'm not
sure why we're storing it in the first place, though; it's queried in exactly
one place, so there isn't really any benefit to precomputing it.
https://github.com/llvm/llvm-p
bwendling wrote:
Thanks! @efriedma-quic anymore comments?
https://github.com/llvm/llvm-project/pull/100272
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -649,6 +649,8 @@ class alignas(void *) Stmt {
/// It is 0 otherwise.
LLVM_PREFERRED_TYPE(bool)
unsigned HasFPFeatures : 1;
+LLVM_PREFERRED_TYPE(bool)
JustinStitt wrote:
Good idea, see
https://github.com/llvm/llvm-project/pull/100272/commits
https://github.com/JustinStitt updated
https://github.com/llvm/llvm-project/pull/100272
>From 154d3505ab13275086b3dffed67bcdcac52f79a3 Mon Sep 17 00:00:00 2001
From: Justin Stitt
Date: Tue, 23 Jul 2024 20:21:49 +
Subject: [PATCH 01/11] implement idiom exclusions
Add flag `-fno-sanitize-ove
@@ -649,6 +649,8 @@ class alignas(void *) Stmt {
/// It is 0 otherwise.
LLVM_PREFERRED_TYPE(bool)
unsigned HasFPFeatures : 1;
+LLVM_PREFERRED_TYPE(bool)
bwendling wrote:
Nit: Newline and maybe a comment?
https://github.com/llvm/llvm-project/pu
@@ -3860,6 +3860,7 @@ class CStyleCastExpr final
class BinaryOperator : public Expr {
enum { LHS, RHS, END_EXPR };
Stmt *SubExprs[END_EXPR];
+ bool ExcludedOverflowPattern = false;
JustinStitt wrote:
Ok, I think I found a spare bit in `BinaryOperatorBits`
@@ -195,13 +196,23 @@ static bool CanElideOverflowCheck(const ASTContext &Ctx,
const BinOpInfo &Op) {
if (!Op.mayHaveIntegerOverflow())
return true;
+ const UnaryOperator *UO = dyn_cast(Op.E);
+
+ if (UO && UO->getOpcode() == UO_Minus && UO->isIntegerConstantExpr(Ctx)
https://github.com/JustinStitt updated
https://github.com/llvm/llvm-project/pull/100272
>From 154d3505ab13275086b3dffed67bcdcac52f79a3 Mon Sep 17 00:00:00 2001
From: Justin Stitt
Date: Tue, 23 Jul 2024 20:21:49 +
Subject: [PATCH 01/10] implement idiom exclusions
Add flag `-fno-sanitize-ove
@@ -3860,6 +3860,7 @@ class CStyleCastExpr final
class BinaryOperator : public Expr {
enum { LHS, RHS, END_EXPR };
Stmt *SubExprs[END_EXPR];
+ bool ExcludedOverflowPattern = false;
efriedma-quic wrote:
I'd prefer to avoid increasing the size of BinaryOper
@@ -195,13 +196,23 @@ static bool CanElideOverflowCheck(const ASTContext &Ctx,
const BinOpInfo &Op) {
if (!Op.mayHaveIntegerOverflow())
return true;
+ const UnaryOperator *UO = dyn_cast(Op.E);
+
+ if (UO && UO->getOpcode() == UO_Minus && UO->isIntegerConstantExpr(Ctx)
https://github.com/bwendling approved this pull request.
LGTM
https://github.com/llvm/llvm-project/pull/100272
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -4759,6 +4759,55 @@ ParenListExpr *ParenListExpr::CreateEmpty(const
ASTContext &Ctx,
return new (Mem) ParenListExpr(EmptyShell(), NumExprs);
}
+namespace {
JustinStitt wrote:
resolved by
https://github.com/llvm/llvm-project/pull/100272/commits/2e3d4795
https://github.com/JustinStitt updated
https://github.com/llvm/llvm-project/pull/100272
>From 154d3505ab13275086b3dffed67bcdcac52f79a3 Mon Sep 17 00:00:00 2001
From: Justin Stitt
Date: Tue, 23 Jul 2024 20:21:49 +
Subject: [PATCH 1/9] implement idiom exclusions
Add flag `-fno-sanitize-overf
@@ -4759,6 +4759,55 @@ ParenListExpr *ParenListExpr::CreateEmpty(const
ASTContext &Ctx,
return new (Mem) ParenListExpr(EmptyShell(), NumExprs);
}
+namespace {
bwendling wrote:
You don't need both an anonymous namespace and marking the function 'static'. I
JustinStitt wrote:
@bwendling I tried to address all your feedback points with
4b3efbb41ff86eeff15671b1d876e1ef6a58a536 Let me know how that looks 👍
https://github.com/llvm/llvm-project/pull/100272
___
cfe-commits mailing list
cfe-commits@lists.llvm.
@@ -2877,6 +2888,17 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const
UnaryOperator *E, LValue LV,
} else if (type->isIntegerType()) {
QualType promotedType;
bool canPerformLossyDemotionCheck = false;
+
+// Is the pattern "while (i--)" and overflow exclusion?
+
@@ -293,6 +293,40 @@ To silence reports from unsigned integer overflow, you can
set
``-fsanitize-recover=unsigned-integer-overflow``, is particularly useful for
providing fuzzing signal without blowing up logs.
+Disabling instrumentation for common overflow patterns
+
@@ -555,6 +570,11 @@ class LangOptions : public LangOptionsBase {
/// The default stream kind used for HIP kernel launching.
GPUDefaultStreamKind GPUDefaultStream;
+ /// Which overflow patterns should be excluded from sanitizer instrumentation
+ int OverflowPatternExclus
@@ -293,6 +293,40 @@ To silence reports from unsigned integer overflow, you can
set
``-fsanitize-recover=unsigned-integer-overflow``, is particularly useful for
providing fuzzing signal without blowing up logs.
+Disabling instrumentation for common overflow patterns
+
@@ -0,0 +1,83 @@
+// Check for potential false positives from patterns that _almost_ match
classic overflow-dependent or overflow-prone code patterns
JustinStitt wrote:
resolved in 4b3efbb41ff86eeff15671b1d876e1ef6a58a536
https://github.com/llvm/llvm-project/pu
@@ -4248,6 +4248,22 @@ bool CompilerInvocation::ParseLangArgs(LangOptions
&Opts, ArgList &Args,
Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
}
+ if (auto *A = Args.getLastArg(OPT_fsanitize_overflow_pattern_exclusion_EQ)) {
+for (int i
https://github.com/JustinStitt updated
https://github.com/llvm/llvm-project/pull/100272
>From 154d3505ab13275086b3dffed67bcdcac52f79a3 Mon Sep 17 00:00:00 2001
From: Justin Stitt
Date: Tue, 23 Jul 2024 20:21:49 +
Subject: [PATCH 1/8] implement idiom exclusions
Add flag `-fno-sanitize-overf
@@ -293,6 +293,40 @@ To silence reports from unsigned integer overflow, you can
set
``-fsanitize-recover=unsigned-integer-overflow``, is particularly useful for
providing fuzzing signal without blowing up logs.
+Disabling instrumentation for common overflow patterns
+
@@ -293,6 +293,40 @@ To silence reports from unsigned integer overflow, you can
set
``-fsanitize-recover=unsigned-integer-overflow``, is particularly useful for
providing fuzzing signal without blowing up logs.
+Disabling instrumentation for common overflow patterns
+
@@ -2877,6 +2888,17 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const
UnaryOperator *E, LValue LV,
} else if (type->isIntegerType()) {
QualType promotedType;
bool canPerformLossyDemotionCheck = false;
+
+// Is the pattern "while (i--)" and overflow exclusion?
+
@@ -555,6 +570,11 @@ class LangOptions : public LangOptionsBase {
/// The default stream kind used for HIP kernel launching.
GPUDefaultStreamKind GPUDefaultStream;
+ /// Which overflow patterns should be excluded from sanitizer instrumentation
+ int OverflowPatternExclus
@@ -0,0 +1,83 @@
+// Check for potential false positives from patterns that _almost_ match
classic overflow-dependent or overflow-prone code patterns
bwendling wrote:
In general, I think it's expected the RUN lines will be first and any comments
about the test
@@ -4248,6 +4248,22 @@ bool CompilerInvocation::ParseLangArgs(LangOptions
&Opts, ArgList &Args,
Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
}
+ if (auto *A = Args.getLastArg(OPT_fsanitize_overflow_pattern_exclusion_EQ)) {
+for (int i
https://github.com/JustinStitt edited
https://github.com/llvm/llvm-project/pull/100272
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
51 matches
Mail list logo
