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
