[PATCH] D21695: [clang] Version support for UBSan handlers

[Filipe Cabecinhas via cfe-commits]

filcab added a comment.

In https://reviews.llvm.org/D21695#510788, @vsk wrote:

> After reading through the discussion in https://reviews.llvm.org/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.


================
Comment at: lib/CodeGen/CGExpr.cpp:2473
@@ +2472,3 @@
+      ("__ubsan_handle_" + CheckName +
+       (CheckInfo.Version ? "_v" + std::to_string(CheckInfo.Version) : "") +
+       (NeedsAbortSuffix ? "_abort" : ""))
----------------
vsk wrote:
> Wdyt of dropping the "_vN" component if the version is 0? That's one less compiler-rt change that we'd need.
That's what it's doing:
  (CheckInfo.Version ? "_v" + std::to_string(CheckInfo.Version) : "")



https://reviews.llvm.org/D21695