lebedev.ri marked 5 inline comments as done.
lebedev.ri added inline comments.
Herald added a subscriber: ychen.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4703-4720
+ // 2) The sign of the difference between the computed address and the base
+ // pointer matches the sign of the total offset.
+ llvm::Value *ValidGEP;
+ auto *NoOffsetOverflow = Builder.CreateNot(OffsetOverflows);
+ if (SignedIndices) {
+ auto *PosOrZeroValid = Builder.CreateICmpUGE(ComputedGEP, IntPtr);
+ auto *PosOrZeroOffset = Builder.CreateICmpSGE(TotalOffset, Zero);
----------------
vsk wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > This makes me ick every time i look at it.
> > > I wonder if this can be sanely rewritten via `.with.overflow` intrinsic..
> > Hm,
> > ```
> > Name: add unsigned
> > %computed = add i8 %base, %offset
> > %PosOrZeroValid = icmp uge i8 %computed, %base
> > =>
> > %agg = uadd_overflow i8 %base, %offset
> > %computed = extractvalue i8 %agg, 0
> > %ov = extractvalue i1 %agg, 1
> > %PosOrZeroValid = xor i1 %ov, -1
> >
> > Name: sub unsigned
> > %computed = add i8 %base, %offset
> > %PosOrZeroValid = icmp ule i8 %computed, %base
> > =>
> > %negated_offset = sub i8 0, %offset
> > %agg = usub_overflow i8 %base, %negated_offset
> > %computed = extractvalue i8 %agg, 0
> > %ov = extractvalue i1 %agg, 1
> > %PosOrZeroValid = xor i1 %ov, -1
> >
> > Name: add signed
> > %computed = add i8 %base, %offset
> > %PosOrZeroValid = icmp uge i8 %computed, %base
> > %PosOrZeroOffset = icmp sge i8 %offset, 0
> > %NegValid = icmp ult i8 %computed, %base
> > %OKay = select i1 %PosOrZeroOffset, i1 %PosOrZeroValid, i1 %NegValid
> > =>
> > %uadd.agg = uadd_overflow i8 %base, %offset
> > %uadd.ov = extractvalue i1 %uadd.agg, 1
> > %negated_offset = sub i8 0, %offset
> > %usub.agg = usub_overflow i8 %base, %negated_offset
> > %usub.ov = extractvalue i1 %usub.agg, 1
> > %computed = add i8 %base, %offset
> > %PosOrZeroOffset = icmp sge i8 %offset, 0
> > %NotOKay = select i1 %PosOrZeroOffset, i1 %uadd.ov, i1 %usub.ov
> > %OKay = xor i1 %NotOKay, -1
> > ```
> >
> > That's not great, but i wonder what is more friendly to middle-end.
> > https://godbolt.org/z/ORSU_L
> I suggest splitting out changes to the existing pointer wraparound check in a
> follow-up patch. This may well be an improvement, but there are enough moving
> parts to this patch as-is.
I'll just precommit them, they are NFC.
================
Comment at: clang/test/CodeGen/catch-nullptr-and-nonzero-offset-blacklist.c:1
+// RUN: %clang_cc1 -fsanitize=pointer-overflow
-fsanitize-recover=pointer-overflow -emit-llvm %s -o - -triple x86_64-linux-gnu
| FileCheck %s -implicit-check-not="call void @__ubsan_handle_pointer_overflow"
--check-prefixes=CHECK,CHECK-NULL-IS-INVALID-PTR
+// RUN: %clang_cc1 -fno-delete-null-pointer-checks -fsanitize=pointer-overflow
-fsanitize-recover=pointer-overflow -emit-llvm %s -o - -triple x86_64-linux-gnu
| FileCheck %s -implicit-check-not="call void @__ubsan_handle_pointer_overflow"
--check-prefixes=CHECK,CHECK-NULL-IS-VALID-PTR
----------------
vsk wrote:
> lebedev.ri wrote:
> > vsk wrote:
> > > Should the test filename be "ignore-nullptr-and-nonzero-..."?
> > `-blacklist.c` is the filename i have i used for all similar testfiles in
> > all previous sanitizers.
> > "black list" means here "list of cases where no sanitization will be
> > emitted".
> This is minor. I only raised the nit as I felt that this test was about
> ignoring "foo", not about catching "foo", and that "blacklist" already means
> something else in the context of sanitizers. Happy to defer to your
> preference here.
Nitpicking is good, less chance to omit something that wasn't intentional and
is thus bad :)
================
Comment at: clang/test/CodeGen/ubsan-pointer-overflow.m:48
void pointer_array_unsigned_indices(int **arr, unsigned k) {
// CHECK: icmp uge
// CHECK-NOT: select
----------------
vsk wrote:
> lebedev.ri wrote:
> > vsk wrote:
> > > I'm curious about what happens here with the new null-status-change
> > > check. If array indices are unsigned, there is no need for a separate
> > > null-status-change check: if the result of the GEP is indeed null, that
> > > will be detected by the pointer wraparound check. We just need to check
> > > that the base pointer itself isn't null.
> > >
> > > That generalizes to addition of unsigned offsets to base pointers, I
> > > think. So e.g. I wouldn't expect the 'preinc' function to contain a
> > > null-status-change check either.
> > Ugh, i still hate those codegen-time optimizations :)
> > Not only do they result in a maze that is 'hard' to comprehend and argue
> > about,
> > the middle-end isn't "forced" to know these folds so the other code stays
> > unoptimized,
> > worse yet, unlike miscompile that will be detected, if the logic error is
> > made here,
> > it will likely "only" result in false-negatives :(
> >
> > > We just need to check that the base pointer itself isn't null.
> >
> > I don't believe that is sound for `unsigned add`:
> > https://rise4fun.com/Alive/2J5 - will consider that `base=0 offset=0` is
> > invalid.
> > Likewise, does not seem valid for `unsigned sub`:
> > https://rise4fun.com/Alive/VEr - will not detect when pointer becomes null.
> > Likewise, does not seem valid for `signed add/sub`:
> > https://rise4fun.com/Alive/97B - will not detect when pointer becomes null.
> >
> > So, i'm not going to touch this. Unless i'm getting the gist of your
> > comment wrong, or wrote those checks wrong?
> Totally see your point and understand the frustration :), we all want a
> tidy/simple frontend. And thanks for sharing these counter-examples. The
> context for my comment here is that -O0 -g and -Os -g compile-time and
> run-time performance (yes! -O0 run-time performance, really) matters a fair
> amount for Xcode users, and tend to regress with sanitizer changes. If there
> are simple (and correct :) ways to avoid regressing these, it's a plus.
I mean, yeah, `-O0` is unreplaceable for //best// debugging expirience, and it
is pretty sluggish.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67122/new/
https://reviews.llvm.org/D67122
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
