vsk added a subscriber: dtzWill.
vsk added a comment.
Thanks, this is looking pretty good.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4538
+ llvm::Value * /*OffsetOverflows*/>
+EmitGEPOffsetInBytes(Value *BasePtr, Value *GEPVal,
+ llvm::LLVMContext &VMContext, CodeGenModule &CGM,
----------------
Please document this helper. Also, consider having it return a struct, so that
the field names don't need to be specified as comments. It'd be great to move
the documentation for the TotalOffset and OffsetOverflows fields in
EmitCheckedInBoundsGEP into the struct definition.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4670
+ OffsetOverflows == Builder.getFalse()) &&
+ "If the offset got constant-folded, we expect that there was no "
+ "overflows.");
----------------
nit: don't expect
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4692
+ // as a result, so the offset can not be -intptr_t(BasePtr).
+ // In other words, both bointers are either null, or both are non-null,
+ // or the behaviour is undefined.
----------------
nit: pointers
================
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
----------------
Should the test filename be "ignore-nullptr-and-nonzero-..."?
================
Comment at: clang/test/CodeGen/catch-nullptr-and-nonzero-offset-blacklist.c:5
+// CHECK-LABEL: @baseline
+char *baseline(char *base, unsigned long offset) {
+ // CHECK: call void @__ubsan_handle_pointer_overflow(
----------------
The baseline case is tested elsewhere, why include it here?
================
Comment at:
clang/test/CodeGen/catch-nullptr-and-nonzero-offset-in-offsetof-idiom.c:1
+// RUN: %clang_cc1 -x c -emit-llvm %s -o - -triple x86_64-linux-gnu |
FileCheck %s
+// RUN: %clang_cc1 -x c -fsanitize=pointer-overflow
-fno-sanitize-recover=pointer-overflow -emit-llvm %s -o - -triple
x86_64-linux-gnu | FileCheck %s
----------------
Should the test filename be "ignore-nullptr-and-nonzero-..."?
================
Comment at:
clang/test/CodeGen/catch-nullptr-and-nonzero-offset-in-offsetof-idiom.c:14
+uintptr_t get_offset_of_y() {
+ // CHECK: define i64 @{{.*}}() {{.*}} {
+ // CHECK-NEXT: [[ENTRY:.*]]:
----------------
nit: CHECK-LABEL: define i64 @get_offset_of_y(
================
Comment at: clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c:12
+// So in other words, the offset can not change "null status" of the pointer,
+// as in if the pointer was null, it can not become non-null, and vice verse,
+// if it was non-null, it can not become null.
----------------
nit: versa
================
Comment at: clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c:28
+
+// We just don't know, can be bad, can be ok.
+char *var_var(char *base, unsigned long offset) {
----------------
I don't quite follow the comments. Perhaps it would be simpler to list the
expected checks in the comment, e.g. "The base and offset are unknown. Check
for pointer wrap and null status change."
================
Comment at: clang/test/CodeGen/catch-nullptr-and-nonzero-offset.c:66
+ // CHECK: define i8* @var_zero_OK(i8* %[[BASE:.*]])
+ // CHECK-NEXT: [[ENTRY:.*]]:
+ // CHECK-NEXT: %[[BASE_ADDR:.*]] = alloca i8*, align 8
----------------
Does this simplify to:
```
CHECK-LABEL: define i8* @var_zero_ok(
CHECK-NOT: !nosanitize
```
================
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
----------------
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.
================
Comment at:
clang/test/CodeGenCXX/catch-nullptr-and-nonzero-offset-in-offsetof-idiom.cpp:1
+// RUN: %clang_cc1 -x c++ -emit-llvm %s -o - -triple x86_64-linux-gnu |
FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsanitize=pointer-overflow
-fno-sanitize-recover=pointer-overflow -emit-llvm %s -o - -triple
x86_64-linux-gnu | FileCheck %s
----------------
How is this different from the other offsetof test file?
================
Comment at:
compiler-rt/test/ubsan/TestCases/Pointer/nullptr-and-nonzero-offset-constants.cpp:22
+ result = base - 1;
+ // CHECK: {{.*}}.cpp:[[@LINE-1]]:17: runtime error: applying non-zero offset
to non-null pointer 0x{{.*}} producing null pointer
+
----------------
Should be 'produced null pointer' or 'gave null result'
================
Comment at:
compiler-rt/test/ubsan_minimal/TestCases/nullptr-and-nonzero-offset.c:1
+// RUN: %clang -fsanitize=pointer-overflow %s -o %t && %run %t 2>&1 |
FileCheck %s --check-prefixes=CHECK
+
----------------
implicit-check-not="pointer-overflow"?
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
