This is an archive of the discontinued LLVM Phabricator instance.

[clang] Version support for UBSan handlers
ClosedPublic

Authored by filcab on Jun 24 2016, 12:05 PM.

Details

Summary

This adds a way for us to version any UBSan handler by itself.
The patch overrides D21289 for a better implementation (we're able to
rev up a single handler).

After this, then we can land a slight modification of D19667+D19668.

We probably don't want to keep all the versions in compiler-rt (maybe we
want to deprecate on one release and remove the old handler on the next
one?), but with this patch we will loudly fail to compile when mixing
incompatible handler calls, instead of silently compiling and then
providing bad error messages.

Diff Detail

Event Timeline

filcab updated this revision to Diff 61817.Jun 24 2016, 12:05 PM
filcab retitled this revision from to [clang] Version support for UBSan handlers.
filcab updated this object.
filcab added reviewers: kcc, samsonov, rsmith.
filcab added a subscriber: cfe-commits.
vsk added a subscriber: vsk.Aug 9 2016, 7:27 PM

After reading through the discussion in D19668, I don't think I understood the pros/cons of using a single ABI check (like asan does) versus adding version numbers to each handler routine. With the latter approach, wouldn't users only be able to link old object files with new runtimes if we never delete old checks? If that's the case, and assuming that we'd like to delete old checks, a single version symbol check would accomplish the same thing.

lib/CodeGen/CGExpr.cpp
2473

Wdyt of dropping the "_vN" component if the version is 0? That's one less compiler-rt change that we'd need.

In D21695#510788, @vsk wrote:

After reading through the discussion in D19668, I don't think I understood the pros/cons of using a single ABI check (like asan does) versus adding version numbers to each handler routine. With the latter approach, wouldn't users only be able to link old object files with new runtimes if we never delete old checks? If that's the case, and assuming that we'd like to delete old checks, a single version symbol check would accomplish the same thing.

With ASan it's very easy to add a single symbol for all of it, since it's a single pass, and when it runs, instrumentation is added. For UBSan it depends on it being enabled and you hitting a place where it checks for it. We can emit the version check symbol in emitCheckHandlerCall, though.

With split versioning, as long as the checks you enable don't get different versions (it's rare that we change checks, too), they'll still work (arguably this is not as valuable as I think it is, but things like Android frameworks enabling UBSan checks in production might make it more valuable).
With a single version for "UBSan", you get more problems:

  • Should you rev when *adding* check handlers?
    • If so, why would I need a newer lib just because it includes a new check, even if I don't use that?
  • You'll need to rev up, and use a newer version of the library even if the checks' interface (for the ones you're using) hasn't changed.
lib/CodeGen/CGExpr.cpp
2473

That's what it's doing:

(CheckInfo.Version ? "_v" + std::to_string(CheckInfo.Version) : "")
vsk added a comment.Aug 12 2016, 11:30 AM
In D21695#510788, @vsk wrote:

After reading through the discussion in D19668, I don't think I understood the pros/cons of using a single ABI check (like asan does) versus adding version numbers to each handler routine. With the latter approach, wouldn't users only be able to link old object files with new runtimes if we never delete old checks? If that's the case, and assuming that we'd like to delete old checks, a single version symbol check would accomplish the same thing.

With ASan it's very easy to add a single symbol for all of it, since it's a single pass, and when it runs, instrumentation is added. For UBSan it depends on it being enabled and you hitting a place where it checks for it. We can emit the version check symbol in emitCheckHandlerCall, though.

Ah, yeah, I can see how this might be cumbersome.

With split versioning, as long as the checks you enable don't get different versions (it's rare that we change checks, too), they'll still work (arguably this is not as valuable as I think it is, but things like Android frameworks enabling UBSan checks in production might make it more valuable).

Running sanitized programs in production sounds strange to me. But, if there isn't really a cost to supporting this, I suppose it's fine.

lib/CodeGen/CGExpr.cpp
2473

Of course! My mistake.

In D21695#514080, @vsk wrote:

Running sanitized programs in production sounds strange to me. But, if there isn't really a cost to supporting this, I suppose it's fine.

It does, and most likely this change wouldn't affect them, as I would guess people running sanitizers in production would probably use sanitize-trap.
But it's being done on parts of Android at least:
https://twitter.com/CopperheadOS/status/717086242966351872

vsk accepted this revision.Nov 29 2016, 11:30 AM
vsk added a reviewer: vsk.

Thanks for working on this. LGTM with a nit.

lib/CodeGen/CGExpr.cpp
2506

Use llvm::array_lengthof? Also, I don't think the >= 0 check is really necessary, but I'll leave it up to you to decide.

This revision is now accepted and ready to land.Nov 29 2016, 11:30 AM
This revision was automatically updated to reflect the committed changes.

This broke the build on android due to use of std::to_string. Would someone mind changing it to llvm::to_string, I don't have commit access to change it myself. Thanks!