This is an archive of the discontinued LLVM Phabricator instance.

ubsan: Unbreak ubsan_cxx runtime library on Windows.
ClosedPublic

Authored by pcc on Sep 14 2017, 7:05 PM.

Details

Summary

This was originally broken by r258744 which introduced a weak reference
from ubsan to ubsan_cxx, which of course does not work directly on
Windows. The fix is to use /alternatename to create a weak external
reference to ubsan_cxx.

Also fix the definition (and the name, so that we drop cached values)
of the cmake flag that controls whether to build ubsan_cxx. Now the
user-controllable flag is always on, and we turn it off internally
depending on whether we support building it.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Sep 14 2017, 7:05 PM
rnk added inline comments.Sep 15 2017, 9:59 AM
compiler-rt/CMakeLists.txt
94 ↗(On Diff #115338)

Is it correct to add OR MSVC to this list, so that the tests requiring cxxabi won't run on Windows?

compiler-rt/lib/ubsan/ubsan_handlers.cc
667–669 ↗(On Diff #115338)

Weak functions strike again.

pcc added inline comments.Sep 15 2017, 11:53 AM
compiler-rt/CMakeLists.txt
94 ↗(On Diff #115338)

The whole point of this change is to make it so that the cxxabi tests do run on Windows :)

Some of the tests use -fsanitize=vptr, which is disabled on Windows because it depends on a working implementation of __ubsan::checkDynamicType, which is not implemented on Windows. However -fsanitize=cfi only uses __ubsan::getDynamicTypeInfoFromVtable which has a Windows implementation.

rnk accepted this revision.Sep 15 2017, 12:55 PM

lgtm

compiler-rt/CMakeLists.txt
94 ↗(On Diff #115338)

I think I misread this: "ubsan_cxx, which of course does not work directly on
Windows" as saying that ubsan_cxx doesn't work on Windows. That's my mistake, the whole point of this change is to make it work better.

This revision is now accepted and ready to land.Sep 15 2017, 12:55 PM
pcc added inline comments.Sep 15 2017, 1:15 PM
compiler-rt/CMakeLists.txt
94 ↗(On Diff #115338)

Yeah, sorry, I'll reword the commit message.

This revision was automatically updated to reflect the committed changes.

Looks like this change changes the user-visible configuration option for cxxabi from SANITIZER_CAN_USE_CXXABI to SANITIZER_ALLOW_CXXABI. Was this intentional/useful? It's not necessarily problematic, but it does cause some annoyances related to maintaining separate build options across compiler-rt branches (5.0 requires SANITIZER_CAN_USE_CXXABI and will not respect SANITIZER_ALLOW_CXXABI, and vice-versa for master/6.0).