[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
filcab added a comment. In https://reviews.llvm.org/D34299#788427, @vsk wrote: > I hope I've cleared this up, but: we need to store the source location > constant _somewhere_, before we emit the return value check. That's because > we can't infer which return location to use at compile time. Yes, sorry about that. I was thinking about the code the wrong way around. I'm ok with this patch and how it got in. Sorry for the delay in replying. Repository: rL LLVM https://reviews.llvm.org/D34299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
This revision was automatically updated to reflect the committed changes.
Closed by commit rL306163: [ubsan] Improve diagnostics for return value checks
(clang) (authored by vedantk).
Changed prior to commit:
https://reviews.llvm.org/D34299?vs=103632=103774#toc
Repository:
rL LLVM
https://reviews.llvm.org/D34299
Files:
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/lib/CodeGen/CGStmt.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.h
cfe/trunk/test/CodeGenObjC/ubsan-nonnull-and-nullability.m
cfe/trunk/test/CodeGenObjC/ubsan-nullability.m
Index: cfe/trunk/test/CodeGenObjC/ubsan-nonnull-and-nullability.m
===
--- cfe/trunk/test/CodeGenObjC/ubsan-nonnull-and-nullability.m
+++ cfe/trunk/test/CodeGenObjC/ubsan-nonnull-and-nullability.m
@@ -7,16 +7,26 @@
// CHECK-LABEL: define nonnull i32* @f1
__attribute__((returns_nonnull)) int *_Nonnull f1(int *_Nonnull p) {
// CHECK: entry:
+ // CHECK-NEXT: [[SLOC_PTR:%.*]] = alloca i8*
// CHECK-NEXT: [[ADDR:%.*]] = alloca i32*
+ // CHECK-NEXT: store i8* null, i8** [[SLOC_PTR]]
// CHECK-NEXT: store i32* [[P:%.*]], i32** [[ADDR]]
+ // CHECK-NEXT: store {{.*}} [[SLOC_PTR]]
// CHECK-NEXT: [[ARG:%.*]] = load i32*, i32** [[ADDR]]
+ // CHECK-NEXT: [[SLOC:%.*]] = load {{.*}} [[SLOC_PTR]]
+ // CHECK-NEXT: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC]], null
+ // CHECK-NEXT: br i1 [[SLOC_NONNULL]], label %nullcheck
+ //
+ // CHECK: nullcheck:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* [[ARG]], null, !nosanitize
// CHECK-NEXT: br i1 [[ICMP]], label %[[CONT:.+]], label %[[HANDLE:[^,]+]]
// CHECK: [[HANDLE]]:
- // CHECK-NEXT: call void @__ubsan_handle_nonnull_return_abort
+ // CHECK: call void @__ubsan_handle_nonnull_return
// CHECK-NEXT: unreachable, !nosanitize
// CHECK: [[CONT]]:
- // CHECK-NEXT: ret i32*
+ // CHECK-NEXT: br label %no.nullcheck
+ // CHECK: no.nullcheck:
+ // CHECK-NEXT: ret i32* [[ARG]]
return p;
}
@@ -29,3 +39,23 @@
// CHECK-NOT: call void @__ubsan_handle_nonnull_arg_abort
f2((void *)0);
}
+
+// If the return value isn't meant to be checked, make sure we don't check it.
+// CHECK-LABEL: define i32* @f3
+int *f3(int *p) {
+ // CHECK-NOT: return.sloc
+ // CHECK-NOT: call{{.*}}ubsan
+ return p;
+}
+
+// Check for a valid "return" source location, even when there is no return
+// statement, to avoid accidentally calling the runtime.
+
+// CHECK-LABEL: define nonnull i32* @f4
+__attribute__((returns_nonnull)) int *f4() {
+ // CHECK: store i8* null, i8** [[SLOC_PTR:%.*]]
+ // CHECK: [[SLOC:%.*]] = load {{.*}} [[SLOC_PTR]]
+ // CHECK: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC]], null
+ // CHECK: br i1 [[SLOC_NONNULL]], label %nullcheck
+ // CHECK: nullcheck:
+}
Index: cfe/trunk/test/CodeGenObjC/ubsan-nullability.m
===
--- cfe/trunk/test/CodeGenObjC/ubsan-nullability.m
+++ cfe/trunk/test/CodeGenObjC/ubsan-nullability.m
@@ -2,31 +2,28 @@
// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s
// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s
-// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 109, i32 1 {{.*}} i32 100, i32 6
+// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 100, i32 6
// CHECK: [[NONNULL_ARG_LOC:@.*]] = private unnamed_addr global {{.*}} i32 204, i32 15 {{.*}} i32 190, i32 23
// CHECK: [[NONNULL_ASSIGN1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 305, i32 9
// CHECK: [[NONNULL_ASSIGN2_LOC:@.*]] = private unnamed_addr global {{.*}} i32 405, i32 10
// CHECK: [[NONNULL_ASSIGN3_LOC:@.*]] = private unnamed_addr global {{.*}} i32 506, i32 10
// CHECK: [[NONNULL_INIT1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 604, i32 25
// CHECK: [[NONNULL_INIT2_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 26
// CHECK: [[NONNULL_INIT2_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 29
-// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 817, i32 1 {{.*}} i32 800, i32 6
+// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 800, i32 6
#define NULL ((void *)0)
#define INULL ((int *)NULL)
#define INNULL ((int *_Nonnull)NULL)
// CHECK-LABEL: define i32* @{{.*}}nonnull_retval1
#line 100
int *_Nonnull nonnull_retval1(int *p) {
- // CHECK: br i1 true, label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
- // CHECK: [[NULL]]:
// CHECK: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
arphaman added a comment. Fair enough. LGTM from my side. https://reviews.llvm.org/D34299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
vsk added a comment. In https://reviews.llvm.org/D34299#788908, @arphaman wrote: > Ok, so now the null check `return.sloc.load` won't call the checker in > compiler-rt and so the program won't `abort` and won't hit the `unreachable`. > I have one question tough: > > This patch changes the behavior of this sanitizer for the example that I gave > above. Yes, in the case where there is no explicit return, and the return value happens to be null. > Previously a runtime diagnostic was emitted, but now there is none. While I'm > not saying that the previous behaviour was correct, I'm wondering if the new > behaviour is right. I think that for C++ it makes sense, but I don't know > the right answer for C. I'm leaning more towards the new behaviour, since > technically in C falling off without returning a value is not UB unless that > return value is used by the caller. But at the same time, since we don't > diagnose `return` UB for C, maybe it's still worth diagnosing this particular > issue? The user might not catch it otherwise at all (or they might catch it > later when they try to access it, but by that point they might not know where > the pointer came from). WDYT? Without seeing what the caller does with the result, the return value check can't do a good job of diagnosing missing returns. I think clang should emit a diagnostic in the example above, and -Wreturn-type does. So I think the new behavior is appropriate. https://reviews.llvm.org/D34299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
arphaman added a comment. Ok, so now the null check `return.sloc.load` won't call the checker in compiler-rt and so the program won't `abort` and won't hit the `unreachable`. I have one question tough: This patch changes the behavior of this sanitizer for the example that I gave above. Previously a runtime diagnostic was emitted, but now there is none. While I'm not saying that the previous behaviour was correct, I'm wondering if the new behaviour is right. I think that for C++ it makes sense, but I don't know the right answer for C. I'm leaning more towards the new behaviour, since technically in C falling off without returning a value is not UB unless that return value is used by the caller. But at the same time, since we don't diagnose `return` UB for C, maybe it's still worth diagnosing this particular issue? The user might not catch it otherwise at all (or they might catch it later when they try to access it, but by that point they might not know where the pointer came from). WDYT? https://reviews.llvm.org/D34299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
vsk added inline comments. Comment at: lib/CodeGen/CGStmt.cpp:1035 +assert(ReturnLocation.isValid() && "No valid return location"); +Builder.CreateStore(Builder.CreateBitCast(SLocPtr, Int8PtrTy), +ReturnLocation); filcab wrote: > Can't you just keep the `Constant*` around and use it later for the static > data? Instead of creating a global var and have runtime store/load? I hope I've cleared this up, but: we need to store the source location constant _somewhere_, before we emit the return value check. That's because we can't infer which return location to use at compile time. Comment at: lib/CodeGen/CodeGenFunction.h:1412 + /// source location for diagnostics. + Address ReturnLocation = Address::invalid(); + filcab wrote: > Maybe `CurrentReturnLocation`? I'd prefer to keep it the way it is, for consistency with the "ReturnValue" field. https://reviews.llvm.org/D34299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
vsk updated this revision to Diff 103632.
vsk marked an inline comment as done.
vsk added a comment.
Handle functions without return statements correctly (fixing an issue pointed
out by @arphaman). Now, the instrumentation always checks that we have a valid
return location before calling the runtime. I added tests for this on the clang
side: we can't test it on the compiler-rt side, because functions without
return statements can cause stack corruption / crashes on Darwin.
https://reviews.llvm.org/D34299
Files:
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGStmt.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGenObjC/ubsan-nonnull-and-nullability.m
test/CodeGenObjC/ubsan-nullability.m
Index: test/CodeGenObjC/ubsan-nullability.m
===
--- test/CodeGenObjC/ubsan-nullability.m
+++ test/CodeGenObjC/ubsan-nullability.m
@@ -2,31 +2,28 @@
// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s
// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s
-// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 109, i32 1 {{.*}} i32 100, i32 6
+// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 100, i32 6
// CHECK: [[NONNULL_ARG_LOC:@.*]] = private unnamed_addr global {{.*}} i32 204, i32 15 {{.*}} i32 190, i32 23
// CHECK: [[NONNULL_ASSIGN1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 305, i32 9
// CHECK: [[NONNULL_ASSIGN2_LOC:@.*]] = private unnamed_addr global {{.*}} i32 405, i32 10
// CHECK: [[NONNULL_ASSIGN3_LOC:@.*]] = private unnamed_addr global {{.*}} i32 506, i32 10
// CHECK: [[NONNULL_INIT1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 604, i32 25
// CHECK: [[NONNULL_INIT2_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 26
// CHECK: [[NONNULL_INIT2_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 29
-// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 817, i32 1 {{.*}} i32 800, i32 6
+// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 800, i32 6
#define NULL ((void *)0)
#define INULL ((int *)NULL)
#define INNULL ((int *_Nonnull)NULL)
// CHECK-LABEL: define i32* @{{.*}}nonnull_retval1
#line 100
int *_Nonnull nonnull_retval1(int *p) {
- // CHECK: br i1 true, label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
- // CHECK: [[NULL]]:
// CHECK: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}[[NONNULL_RV_LOC1]]
return p;
- // CHECK: [[NONULL]]:
- // CHECK-NEXT: ret i32*
+ // CHECK: ret i32*
}
#line 190
@@ -108,10 +105,13 @@
// CHECK-NEXT: [[DO_RV_CHECK_1:%.*]] = and i1 true, [[ARG1CMP]], !nosanitize
// CHECK: [[ARG2CMP:%.*]] = icmp ne i32* %arg2, null, !nosanitize
// CHECK-NEXT: [[DO_RV_CHECK_2:%.*]] = and i1 [[DO_RV_CHECK_1]], [[ARG2CMP]]
- // CHECK: br i1 [[DO_RV_CHECK_2]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
+ // CHECK: [[SLOC_PTR:%.*]] = load i8*, i8** %return.sloc.ptr
+ // CHECK-NEXT: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC_PTR]], null
+ // CHECK-NEXT: [[DO_RV_CHECK_3:%.*]] = and i1 [[SLOC_NONNULL]], [[DO_RV_CHECK_2]]
+ // CHECK: br i1 [[DO_RV_CHECK_3]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}[[NONNULL_RV_LOC2]]
return arg1;
// CHECK: [[NONULL]]:
@@ -129,10 +129,13 @@
+(int *_Nonnull) objc_clsmethod: (int *_Nonnull) arg1 {
// CHECK: [[ARG1CMP:%.*]] = icmp ne i32* %arg1, null, !nosanitize
// CHECK-NEXT: [[DO_RV_CHECK:%.*]] = and i1 true, [[ARG1CMP]]
- // CHECK: br i1 [[DO_RV_CHECK]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
+ // CHECK: [[SLOC_PTR:%.*]] = load i8*, i8** %return.sloc.ptr
+ // CHECK-NEXT: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC_PTR]], null
+ // CHECK-NEXT: [[DO_RV_CHECK_2:%.*]] = and i1 [[SLOC_NONNULL]], [[DO_RV_CHECK]]
+ // CHECK: br i1 [[DO_RV_CHECK_2]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}
return arg1;
// CHECK: [[NONULL]]:
@@ -143,10 +146,13 @@
-(int *_Nonnull) objc_method: (int *_Nonnull) arg1 {
// CHECK:
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
vsk added a comment. In https://reviews.llvm.org/D34299#788379, @filcab wrote: > In https://reviews.llvm.org/D34299#788152, @vsk wrote: > > > The source locations aren't constants. The ubsan runtime uses a bit inside > > of source location structures as a flag. When an issue is diagnosed at a > > particular source location, that bit is atomically set. This is how ubsan > > implements issue deduplication. > > > It's still an `llvm::Constant`. Just like in StaticData, in line 2966. > Basically, I don't see why we need to add the store/load and an additional > indirection, since the pointer is constant, and we can just emit the static > data as before. My earlier response made the assumption that you wanted a copy of the source location passed to the runtime handler, by-value. I see now that you're wondering why the source locations aren't part of the static data structure. That's because the location of the return statement isn't known at compile time, i.e it's not static data. The location depends on which return statement is executed. > We're already doing `Data->Loc.acquire();` for the current version (and all > the other checks). The other checks do not allow the source location within a static data object to change. >>> I'd also like to have some asserts and explicit resets to nullptr after use >>> on the ReturnLocation variable, if possible. >> >> Resetting Address fields in CodeGenFunction doesn't appear to be an >> established practice. Could you explain what this would be in aid of? > > It would be a sanity check and help with code reading/keeping in mind the > lifetime of the information. I'm ok with that happening only on `!NDEBUG` > builds. > > Reading the code, I don't know if a `ReturnLocation` might end up being used > for more than one checks. If it's supposed to be a different one per check, > etc. > If it's only supposed to be used once, I'd rather set it to `nullptr` right > after use (at least when `!NDEBUG`), and `assert(!ReturnLocation)` before > setting. It's not a big deal, but would help with making sense of the flow of > information when debugging. Sure, I'll reset it to Address::invalid(). https://reviews.llvm.org/D34299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
filcab added a comment. In https://reviews.llvm.org/D34299#788152, @vsk wrote: > The source locations aren't constants. The ubsan runtime uses a bit inside of > source location structures as a flag. When an issue is diagnosed at a > particular source location, that bit is atomically set. This is how ubsan > implements issue deduplication. It's still an `llvm::Constant`. Just like in StaticData, in line 2966. Basically, I don't see why we need to add the store/load and an additional indirection, since the pointer is constant, and we can just emit the static data as before. We're already doing `Data->Loc.acquire();` for the current version (and all the other checks). >> I'd also like to have some asserts and explicit resets to nullptr after use >> on the ReturnLocation variable, if possible. > > Resetting Address fields in CodeGenFunction doesn't appear to be an > established practice. Could you explain what this would be in aid of? It would be a sanity check and help with code reading/keeping in mind the lifetime of the information. I'm ok with that happening only on `!NDEBUG` builds. Reading the code, I don't know if a `ReturnLocation` might end up being used for more than one checks. If it's supposed to be a different one per check, etc. If it's only supposed to be used once, I'd rather set it to `nullptr` right after use (at least when `!NDEBUG`), and `assert(!ReturnLocation)` before setting. It's not a big deal, but would help with making sense of the flow of information when debugging. https://reviews.llvm.org/D34299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
arphaman added a comment.
In https://reviews.llvm.org/D34299#788152, @vsk wrote:
> In https://reviews.llvm.org/D34299#787795, @arphaman wrote:
>
> > It looks like if we have a function without the `return` (like the sample
> > below), we will pass in a `0` as the location pointer. This will prevent a
> > report of a runtime error as your compiler-rt change ignores the location
> > pointers that are `nil`. Is this a bug or is this the intended behaviour?
> >
> > int *_Nonnull nonnull_retval1(int *p) {
> > }
> >
>
>
> This is the intended behavior (I'll add a test). Users should not see a "null
> return value" diagnostic here. There is another check, -fsanitize=return,
> which can catch this issue.
Ok. However, when the source is not using C++, the check for
`-fsanitize=return` won't be emitted. So we will end up with a call to
`__ubsan_handle_nullability_return_abort` without a location, which won't
report a diagnostic, but the program will crash because compiler-rt will call
`abort`. This seems like a regression, since previously in C mode this
diagnostic was reported.
> @filcab --
>
>> Splitting the attrloc from the useloc might make sense since we would be
>> able to emit attrloc just once. But I don't see why we need to store/load
>> those pointers in runtime instead of just caching the Constant* in
>> CodeGenFunction.
>
> The source locations aren't constants. The ubsan runtime uses a bit inside of
> source location structures as a flag. When an issue is diagnosed at a
> particular source location, that bit is atomically set. This is how ubsan
> implements issue deduplication.
>
>> I'd also like to have some asserts and explicit resets to nullptr after use
>> on the ReturnLocation variable, if possible.
>
> Resetting Address fields in CodeGenFunction doesn't appear to be an
> established practice. Could you explain what this would be in aid of?
https://reviews.llvm.org/D34299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
vsk added a comment.
In https://reviews.llvm.org/D34299#787795, @arphaman wrote:
> It looks like if we have a function without the `return` (like the sample
> below), we will pass in a `0` as the location pointer. This will prevent a
> report of a runtime error as your compiler-rt change ignores the location
> pointers that are `nil`. Is this a bug or is this the intended behaviour?
>
> int *_Nonnull nonnull_retval1(int *p) {
> }
>
This is the intended behavior (I'll add a test). Users should not see a "null
return value" diagnostic here. There is another check, -fsanitize=return, which
can catch this issue.
@filcab --
> Splitting the attrloc from the useloc might make sense since we would be able
> to emit attrloc just once. But I don't see why we need to store/load those
> pointers in runtime instead of just caching the Constant* in CodeGenFunction.
The source locations aren't constants. The ubsan runtime uses a bit inside of
source location structures as a flag. When an issue is diagnosed at a
particular source location, that bit is atomically set. This is how ubsan
implements issue deduplication.
> I'd also like to have some asserts and explicit resets to nullptr after use
> on the ReturnLocation variable, if possible.
Resetting Address fields in CodeGenFunction doesn't appear to be an established
practice. Could you explain what this would be in aid of?
https://reviews.llvm.org/D34299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
filcab added a comment. Splitting the attrloc from the useloc might make sense since we would be able to emit attrloc just once. But I don't see why we need to store/load those pointers in runtime instead of just caching the `Constant*` in `CodeGenFunction`. I'd also like to have some asserts and explicit resets to `nullptr` after use on the `ReturnLocation` variable, if possible. Comment at: lib/CodeGen/CGStmt.cpp:1035 +assert(ReturnLocation.isValid() && "No valid return location"); +Builder.CreateStore(Builder.CreateBitCast(SLocPtr, Int8PtrTy), +ReturnLocation); Can't you just keep the `Constant*` around and use it later for the static data? Instead of creating a global var and have runtime store/load? Comment at: lib/CodeGen/CodeGenFunction.h:1412 + /// source location for diagnostics. + Address ReturnLocation = Address::invalid(); + Maybe `CurrentReturnLocation`? https://reviews.llvm.org/D34299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
arphaman added a comment.
It looks like if we have a function without the `return` (like the sample
below), we will pass in a `0` as the location pointer. This will prevent a
report of a runtime error as your compiler-rt change ignores the location
pointers that are `nil`. Is this a bug or is this the intended behaviour?
int *_Nonnull nonnull_retval1(int *p) {
}
https://reviews.llvm.org/D34299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34299: [ubsan] Improve diagnostics for return value checks (clang)
vsk created this revision.
This patch makes ubsan's nonnull return value diagnostics more precise,
which makes the diagnostics more useful when there are multiple return
statements in a function. Example:
1 |__attribute__((returns_nonnull)) char *foo() {
2 | if (...) {
3 |return expr_which_might_evaluate_to_null();
4 | } else {
5 |return another_expr_which_might_evaluate_to_null();
6 | }
7 |} // <- The current diagnostic always points here!
runtime error: Null returned from Line 7, Column 2!
With this patch, the diagnostic would point to either Line 3, Column 5
or Line 5, Column 5.
This is done by emitting source location metadata for each return
statement in a sanitized function. The runtime is passed a pointer to
the appropriate metadata so that it can prepare and deduplicate reports.
Compiler-rt patch (with more tests): https://reviews.llvm.org/D34298
https://reviews.llvm.org/D34299
Files:
lib/CodeGen/CGCall.cpp
lib/CodeGen/CGStmt.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGenObjC/ubsan-nonnull-and-nullability.m
test/CodeGenObjC/ubsan-nullability.m
Index: test/CodeGenObjC/ubsan-nullability.m
===
--- test/CodeGenObjC/ubsan-nullability.m
+++ test/CodeGenObjC/ubsan-nullability.m
@@ -2,15 +2,15 @@
// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s
// RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s
-// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 109, i32 1 {{.*}} i32 100, i32 6
+// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 100, i32 6
// CHECK: [[NONNULL_ARG_LOC:@.*]] = private unnamed_addr global {{.*}} i32 204, i32 15 {{.*}} i32 190, i32 23
// CHECK: [[NONNULL_ASSIGN1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 305, i32 9
// CHECK: [[NONNULL_ASSIGN2_LOC:@.*]] = private unnamed_addr global {{.*}} i32 405, i32 10
// CHECK: [[NONNULL_ASSIGN3_LOC:@.*]] = private unnamed_addr global {{.*}} i32 506, i32 10
// CHECK: [[NONNULL_INIT1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 604, i32 25
// CHECK: [[NONNULL_INIT2_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 26
// CHECK: [[NONNULL_INIT2_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 29
-// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 817, i32 1 {{.*}} i32 800, i32 6
+// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 800, i32 6
#define NULL ((void *)0)
#define INULL ((int *)NULL)
@@ -22,7 +22,7 @@
// CHECK: br i1 true, label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}[[NONNULL_RV_LOC1]]
return p;
// CHECK: [[NONULL]]:
@@ -111,7 +111,7 @@
// CHECK: br i1 [[DO_RV_CHECK_2]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}[[NONNULL_RV_LOC2]]
return arg1;
// CHECK: [[NONULL]]:
@@ -132,7 +132,7 @@
// CHECK: br i1 [[DO_RV_CHECK]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}
return arg1;
// CHECK: [[NONULL]]:
@@ -146,7 +146,7 @@
// CHECK: br i1 [[DO_RV_CHECK]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize
// CHECK: [[NULL]]:
// CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize
- // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize
+ // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
// CHECK: call void @__ubsan_handle_nullability_return{{.*}}
return arg1;
// CHECK: [[NONULL]]:
Index: test/CodeGenObjC/ubsan-nonnull-and-nullability.m
===
--- test/CodeGenObjC/ubsan-nonnull-and-nullability.m
+++ test/CodeGenObjC/ubsan-nonnull-and-nullability.m
@@ -8,12 +8,16 @@
__attribute__((returns_nonnull)) int *_Nonnull f1(int *_Nonnull p) {
// CHECK: entry:
// CHECK-NEXT: [[ADDR:%.*]] = alloca i32*
+ // CHECK-NEXT: [[SLOC_PTR:%.*]] = alloca i8*
+ // CHECK-NEXT: store i8* null, i8** [[SLOC_PTR]]
// CHECK-NEXT: store
