This is an archive of the discontinued LLVM Phabricator instance.

[ubsan] Improve diagnostics for return value checks (compiler-rt)
ClosedPublic

Authored by vsk on Jun 16 2017, 3:19 PM.

Diff Detail

Event Timeline

vsk created this revision.Jun 16 2017, 3:19 PM
arphaman added inline comments.
lib/ubsan/ubsan_interface.inc
31

Just to confirm: We don't care about backwards compatibility, and the old version can be dropped safely?

filcab added inline comments.Jun 22 2017, 5:35 AM
lib/ubsan/ubsan_handlers.cc
479

This looks fishy. Can you add a comment to explain why this might happen?

lib/ubsan/ubsan_interface.inc
31

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).

test/ubsan/TestCases/Misc/nonnull.cpp
15

Shouldn't you do a similar transformation for the nullability test?

vsk added inline comments.Jun 22 2017, 10:43 AM
lib/ubsan/ubsan_interface.inc
31

@arphaman Yes, we're intentionally dropping backwards compatibility here. Xcode's build system knows to package up sanitizer runtimes into app bundles, so back/future-deployment isn't a concern. That is also not the common/supported use case -- runtime tools are generally meant for testing.

test/ubsan/TestCases/Misc/nonnull.cpp
15

I did update the nullability.c test. If you mean that we should have a function that looks like "bar", but with a _Nonnull return value annotation, I can add that in.

filcab added inline comments.Jun 22 2017, 1:36 PM
test/ubsan/TestCases/Misc/nonnull.cpp
15

yeah, I guess there's no need for an additional one here, since we can tell it's emitting at the new place (expression for return), and you should already have the call emission covered in the clang part.

vsk updated this revision to Diff 103635.Jun 22 2017, 2:25 PM
vsk marked 4 inline comments as done.
vsk edited the summary of this revision. (Show Details)

Make the null check in the non-null return value handler an unexpected case.

This revision was automatically updated to reflect the committed changes.