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.
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
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
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
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.
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:
>
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
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
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
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
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
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
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
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
14 matches
Mail list logo
