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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

Looks great with some nits! Checking `_ZTS` in `TypeIDVisibleToRegularObj` (switch to `typeIDVisibleToRegularObj`) looks good to me. Sorry for the delay.
There are a number of resolved comments you may want to mark as done.



================
Comment at: lld/ELF/Driver.cpp:1026
+static void ltoValidateAllVtablesHaveTypeInfos(opt::InputArgList &args) {
+  llvm::DenseSet<StringRef> typeInfoSymbols;
+  llvm::DenseSet<StringRef> vtableSymbols;
----------------
The conventional style in lld/ omits `llvm::` for DenseSet.


================
Comment at: lld/ELF/Driver.cpp:1035
+
+  // Examine all native symbol tables
+  for (ELFFileBase *f : ctx.objectFiles) {
----------------



================
Comment at: lld/ELF/Driver.cpp:1057
+  SmallSetVector<StringRef, 0> vtableSymbolsWithNoRTTI;
+  for (auto &s : vtableSymbols)
+    if (!typeInfoSymbols.count(s))
----------------
This still relies on the iteration order of `DenseSet vtableSymbols`. We need SetVector for `vtableSymbols` as well.

`auto &s` => `StringRef s`


================
Comment at: lld/ELF/Driver.cpp:2848
 
+  // Handle -lto-validate-all-vtables-had-type-infos
+  if (config->ltoValidateAllVtablesHaveTypeInfos)
----------------
MaskRay wrote:
> 
Not done.


================
Comment at: lld/ELF/Options.td:601
   "turn off warnings about profile cfg mismatch">;
+defm lto_known_safe_vtables : Eq<"lto-known-safe-vtables", "When --lto-validate-all-vtables-have-type-infos is enabled, skip validation on these vtables">;
 def lto_obj_path_eq: JJ<"lto-obj-path=">;
----------------
MaskRay wrote:
> New options use `EEq` to disallow single-dash long options, to not conflict with `-l`.
`When --lto-validate-all-vtables-have-type-infos is enabled, skip validation on these vtables (_ZTV symbols)`


================
Comment at: llvm/lib/LTO/LTO.cpp:1298
+  auto IsVisibleToRegularObj = [&](StringRef name) {
+    return GlobalResolutions.count(name)
+               ? GlobalResolutions[name].VisibleOutsideSummary
----------------
redundant hash table lookup here.

Better to use `find(name)` with slightly more code


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:787
 
+bool TypeIDVisibleToRegularObj(
+    StringRef TypeID, function_ref<bool(StringRef)> IsVisibleToRegularObj) {
----------------
Perhaps we should fix these functions to follow https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions . Pushed b4d4146db3b9a29773259c8b8a6cb7c98da90e73 and you'll need a rebase.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:798
+  // external native files. See CodeGenModule::CreateMetadataIdentifierImpl.
+  if (!TypeID.starts_with("_ZTS"))
+    return false;
----------------
Use `consume_front` here to avoid `consume_front` below.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:807
+  TypeID.consume_front("_ZTS");
+  std::string typeInfo = "_ZTI" + TypeID.str();
+  return IsVisibleToRegularObj(typeInfo);
----------------
to avoid constructing a possibly heap-allocated std::string twice.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:811
+
+bool SkipUpdateDueToValidation(
+    GlobalVariable &GV, function_ref<bool(StringRef)> IsVisibleToRegularObj) {
----------------
`skipUpdateDueToValidation`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155659