[PATCH] D34298: [ubsan] Improve diagnostics for return value checks (compiler-rt)

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: lib/ubsan/ubsan_handlers.cc:479
+  if (!LocPtr)
+    return;
+
----------------
This looks fishy. Can you add a comment to explain why this might happen?


================
Comment at: lib/ubsan/ubsan_interface.inc:31
 INTERFACE_FUNCTION(__ubsan_handle_nonnull_arg_abort)
-INTERFACE_FUNCTION(__ubsan_handle_nonnull_return)
-INTERFACE_FUNCTION(__ubsan_handle_nonnull_return_abort)
+INTERFACE_FUNCTION(__ubsan_handle_nonnull_return_v1)
+INTERFACE_FUNCTION(__ubsan_handle_nonnull_return_v1_abort)
----------------
arphaman wrote:
> Just to confirm: We don't care about backwards compatibility, and the old version can be dropped safely?
The usual reasoning is that we, in open source, always want to have matching clang+compiler-rt. But we don't want mysterious problems when mismatches happen, so I introduced the `_v*` mechanism to force the linker to "yell loudly" when there's a mismatch.

In the past, the first change to check data I remember (after the check has been in an llvm release) was made by me and we managed to come up with a heuristic to avoid revving the checks. The second change introduced the `_v*` mechanism (and also removed the older symbol).


================
Comment at: test/ubsan/TestCases/Misc/nonnull.cpp:15
+
+__attribute__((returns_nonnull)) char *bar(int x, char *a) {
+  if (x > 10) {
----------------
Shouldn't you do a similar transformation for the nullability test?


https://reviews.llvm.org/D34298