[PATCH] D155659: [WPD][LLD] Add option to validate RTTI is enabled on all native types and prevent devirtualization on types with native RTTI

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D155659#4640392 <https://reviews.llvm.org/D155659#4640392>, @MaskRay wrote:

> In D155659#4640159 <https://reviews.llvm.org/D155659#4640159>, @modimo wrote:
>
>> Gentle ping @MaskRay
>
> Sorry for the delay. (There were Phabricator <https://discourse.llvm.org/t/phabricator-is-very-slow/73132/9?u=maskray> issues <https://discourse.llvm.org/t/reviews-llvm-org-read-only-mode/73289> to handle beside work...)
>
> I will need to re-read https://discourse.llvm.org/t/rfc-safer-whole-program-class-hierarchy-analysis/65144 and an internal discussion in May 2022 when I was thinking about `_ZTI`.
>
> `-frtti` is discouraged by https://google.github.io/styleguide/cppguide.html#Run-Time_Type_Information__RTTI_ , so I think it may not benefit us... but this feature is still useful. I need to read these discussions...

I looked at this more closely after this patch was mailed. While use of RTTI is discouraged, building with -fno-rtti isn't specifically encouraged afaict, and when I built a large number of important binaries internally with this patch in validation mode, it turned out that there were only a few objects/symbols affected, and we could allowlist them to enable WPD. See my earlier comment from July 25:

> I cranked through a bunch of builds with this change and thankfully while they all do have at least one vtable from an -fno-rtti native object, there are only a handful of unique symbols (which all appear safe), so we could consider using --lto-known-safe-vtables to allowlist them.



> I wonder why the following example is still incorrect.

Hmm, that seems like exactly the case that should be caught and handled automatically by this patch. Oh, I just compiled the same source code to a native object and nm shows a reference to the typeinfo for A, but no type name for A (_ZTS1A):

  $ nm b.o
                   U _Z3barP1A
  0000000000000000 T _Z3bazv
  0000000000000000 W _ZN1B3fooEv
                   U _ZTI1A
  0000000000000000 V _ZTI1B
  0000000000000000 V _ZTS1B
  0000000000000000 V _ZTV1B
                   U _ZTVN10__cxxabiv120__si_class_type_infoE

The _ZTS symbol is the one referenced by the type metadata, and what this patch is going to look for being referenced in native objects. Not familiar with the rules around when that symbol ends up being used, but if it isn't consistently referenced from native objects then this solution isn't going to work as well as hoped. Should it be looking for either the type name or the corresponding type info?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155659/new/

https://reviews.llvm.org/D155659