[PATCH] D38599: Remove warnings for dynamic_cast fallback.

[Ryan Prichard via Phabricator via cfe-commits]

rprichard added a comment.

In https://reviews.llvm.org/D38599#899198, @danalbert wrote:

> In https://reviews.llvm.org/D38599#899196, @howard.hinnant wrote:
>
> > Fwiw, I wrote this code.  All of that "fallback" stuff was written to make customer code that was incorrect, but working on OS X -version-that-used-libsupc++ continue to work.  I.e. to be a crutch for incorrect code.  It should all be removed if you no longer want to provide such a crutch.
>
>
> Android may end up wanting to use it for the same reason. I've backed it out for now, but it may end up being something we need since a fair number of NDK users (majority, probably) use libsupc++ and that might be a point of pain in migrating to libc++abi. Other Linux distros may want it as well, so probably worth leaving the code around.


On OS X, dynamic vague linkage appears to work by default, and _LIBCXX_DYNAMIC_FALLBACK catches some code that slips through the cracks (e.g. code that does something odd like: specify visibility attributes, explicitly specify RTLD_LOCAL, link with -Bsymbolic, etc.). dlopen defaults to RTLD_GLOBAL, which merges weak definitions, even with an executable. System.loadLibrary uses RTLD_GLOBAL.

On Android, AFAICT, dynamic vague linkage doesn't work prior to M, and with M, it only works between shared objects loaded in one batch. Loading multiple C++ shared objects one-at-a-time generally results in duplicated type_info objects, which break the various RTTI-dependent features when libc++/libc++abi are used.

_LIBCXX_DYNAMIC_FALLBACK doesn't generally fix __dynamic_cast to account for duplicated type_info objects. Instead, it flags a specific situation that's easy to detect. If we have code like this...

  Static *s = new Derived;
  dynamic_cast<Dest*>(s)

... __dynamic_cast will search the RTTI graph of `Derived` looking for `Static` and `Dest`. It expects to find at least one instance of `Static`. If it doesn't, there's obviously something wrong, because, assuming the C++ ABI has been followed, the `s` pointer is not really of type `Static`! This situation is cheap to detect (a couple extra comparisons). If `Static` //is// found, the fallback doesn't activate, but dynamic_cast can still fail, e.g.:

- If a `Dest` in the RTTI graph doesn't match `&typeid(Dest)` where dynamic_cast is used, then dynamic_cast can incorrectly return NULL.
- If there are two `Dest` objects in the RTTI graph with unequal addresses, then dynamic_cast can incorrectly return non-NULL when the cast should have been ambiguous. (Writing this test case is a fun little exercise.)
- Multiple `Static` objects with unequal addresses probably causes similar failures, as long as one of them matches `&typeid(Static)` where dynamic_cast is used.

I think _LIBCXX_DYNAMIC_FALLBACK is fine to enable on Android, but (1) it seems insufficient, (2) I'm skeptical about removing the diagnostic, and (3) a general solution to the libc++abi RTTI problem tends to make _LIBCXX_DYNAMIC_FALLBACK irrelevant. i.e. We may need to simply compare typeinfo strings like gnustl does (or the `#ifdef _WIN32` code for dynamic_cast), which subsumes _LIBCXX_DYNAMIC_FALLBACK.

I'm still intrigued by the `shouldRTTIBeUnique` / `_LIBCPP_NONUNIQUE_RTTI_BIT` stuff I linked above. I'm a little surprised that 64-bit iOS would need that behavior. Maybe we need to turn it on for Android, but where is the corresponding flag for libcxxabi?


https://reviews.llvm.org/D38599