This revision was automatically updated to reflect the committed changes.
Closed by commit rL296213: [ubsan] Omit superflous overflow checks for promoted
arithmetic (PR20193) (authored by vedantk).
Changed prior to commit:
https://reviews.llvm.org/D29369?vs=89733=89752#toc
Repository:
rL
dtzWill accepted this revision.
dtzWill added a comment.
This revision is now accepted and ready to land.
Sorry for the delay!
LGTM, thanks!
https://reviews.llvm.org/D29369
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
vsk updated this revision to Diff 89733.
vsk added a comment.
- Make the suggested readability improvements, and fix a comment in the test
case.
https://reviews.llvm.org/D29369
Files:
lib/CodeGen/CGExprScalar.cpp
test/CodeGen/compound-assign-overflow.c
vsk added a comment.
Ping, is the argument in favor of making the change in my last comment
satisfactory?
https://reviews.llvm.org/D29369
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
vsk added a comment.
In https://reviews.llvm.org/D29369#676521, @dtzWill wrote:
> In https://reviews.llvm.org/D29369#673064, @vsk wrote:
>
> > In https://reviews.llvm.org/D29369#672166, @dtzWill wrote:
> >
> > > After some thought, can we discuss why this is a good idea?
> >
> >
> > The goal is
dtzWill added a comment.
In https://reviews.llvm.org/D29369#673064, @vsk wrote:
> In https://reviews.llvm.org/D29369#672166, @dtzWill wrote:
>
> > After some thought, can we discuss why this is a good idea?
>
>
> The goal is to lower ubsan's compile-time + instrumentation overhead at -O0,
>
vsk added a comment.
In https://reviews.llvm.org/D29369#672166, @dtzWill wrote:
> After some thought, can we discuss why this is a good idea?
The goal is to lower ubsan's compile-time + instrumentation overhead at -O0,
since this reduces the friction of debugging a ubsan-instrumented project.
dtzWill requested changes to this revision.
dtzWill added a comment.
This revision now requires changes to proceed.
After some thought, can we discuss why this is a good idea?
This increases the cyclomatic complexity of code that already is difficult to
reason about, and seems like it's both
dtzWill accepted this revision.
dtzWill added a comment.
I've been bitten when attempting to use existence/nature of casts in the AST to
reason about the original code, but this looks like it does the right thing in
all the situations I can think of.
Missing overflows because of a bugged
regehr added a comment.
Paging @dtzWill
https://reviews.llvm.org/D29369
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
filcab added a comment.
Minor nits, now.
LGTM, but having someone more familiar with clang chime in would be great.
Comment at: lib/CodeGen/CGExprScalar.cpp:1700
case LangOptions::SOB_Trapping:
+if (getUnwidenedIntegerType(CGF.getContext(), E->getSubExpr()).hasValue())
vsk updated this revision to Diff 87030.
vsk edited the summary of this revision.
vsk added a comment.
- Use switches per Filipe's comment, and fix a comment in the test case.
https://reviews.llvm.org/D29369
Files:
lib/CodeGen/CGExprScalar.cpp
test/CodeGen/compound-assign-overflow.c
vsk marked an inline comment as done.
vsk added a comment.
In https://reviews.llvm.org/D29369#666308, @vsk wrote:
> In https://reviews.llvm.org/D29369#665878, @filcab wrote:
>
> > Why the switch to `if` instead of a fully-covered switch/case?
>
>
> It lets me avoid repeating two function calls:
filcab added a comment.
Why the switch to `if` instead of a fully-covered switch/case?
In https://reviews.llvm.org/D29369#664426, @vsk wrote:
> In https://reviews.llvm.org/D29369#664366, @regehr wrote:
>
> > Out of curiosity, how many of these superfluous checks are not subsequently
> >
vsk added a comment.
In https://reviews.llvm.org/D29369#664366, @regehr wrote:
> Out of curiosity, how many of these superfluous checks are not subsequently
> eliminated by InstCombine?
I don't have numbers from a benchmark prepped. Here's what we get with the
'ubsan-promoted-arith.cpp' test
regehr added a comment.
Out of curiosity, how many of these superfluous checks are not subsequently
eliminated by InstCombine?
https://reviews.llvm.org/D29369
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
vsk updated this revision to Diff 86746.
vsk edited the summary of this revision.
vsk added a comment.
- Remove a stale test case in unsigned-promotion.c.
https://reviews.llvm.org/D29369
Files:
lib/CodeGen/CGExprScalar.cpp
test/CodeGen/compound-assign-overflow.c
vsk updated this revision to Diff 86739.
vsk marked an inline comment as done.
vsk added a comment.
- Per Eli's comment: check that integers are actually widened, instead of
incorrectly assuming they are always widened. I added some test cases for this.
- Address the 'fixme' regarding
vsk marked an inline comment as done.
vsk added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:72
+ if (const auto *UO = dyn_cast(Op.E))
+return IsPromotedInteger(UO->getSubExpr());
+
efriedma wrote:
> Checking isPromotableIntegerType doesn't
efriedma added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:72
+ if (const auto *UO = dyn_cast(Op.E))
+return IsPromotedInteger(UO->getSubExpr());
+
Checking isPromotableIntegerType doesn't work the way you want it to; types can
be "promoted"
vsk created this revision.
C requires the operands of arithmetic expressions to be promoted if
their types are smaller than an int. Ubsan emits overflow checks when
this sort of type promotion occurs, even if there is no way to actually
get an overflow with the promoted type.
This patch teaches
21 matches
Mail list logo
