pcc added a comment.
It doesn't seem sound to create the distinction between translation unit and
linkage unit visibility and then allow passes to relax visibility. For example,
imagine that you have an executable with (compiled with `-fvisibility=default`):
struct PluginInterface {
virtual void foo() = 0;
}
struct Plugin1 : PluginInterface {
virtual void foo();
};
void Plugin1::foo() {
// ...
}
And a DSO loaded with dlopen:
void call_foo(PluginInterface *plugin) {
plugin->foo();
}
Note that the lack of attribute means that the vtables have public LTO
visibility. Depending on the contents of the DSO, the `PluginInterface` and
`Plugin1` vtable symbols may not be referenced at all by the DSO. If the DSO is
loaded via `dlopen` it will definitely not be referenced at link time. If we
allow LTO to relax `PluginInterface` and `Plugin1` vtables to what you're
calling `VCallVisibilityTranslationUnit` in the main executable and there are
no calls to `foo()` in the main executable, we will incorrectly drop the
definition of `Plugin1::foo()`.
The approach that I can see working involves basically the same structure as
CFI and WPD: the LTO visibility is decided by the frontend and not relaxed by
passes or LTO. There is only one "flavour" of `!vcall_visibility` metadata: the
one that means the entire hierarchy below the vtable has hidden LTO visibility.
> However, the
> changes in clang to use the llvm.type.checked.load intrinsic are causing ~1%
> performance regression in the C++ parts of SPEC2006.
Have you checked the assembly to see whether an unused CFI check is being
emitted?
================
Comment at: clang/lib/CodeGen/CGClass.cpp:2816
+ } else {
+ Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::assume), CheckResult);
+ }
----------------
There should be no need to emit this `llvm.assume` intrinsic. We can rely on
the unspecified behaviour in the case where the second element returned by
`llvm.type.checked.load` is false.
================
Comment at: llvm/docs/TypeMetadata.rst:240
+
+ __attribute__((visibility("public")))
+ struct A {
----------------
`s/"public"/"default"/`
================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:183
+ // unit, we know that we can see all virtual functions which might use it,
+ // so VFE is safe.
+ if (auto GO = dyn_cast<GlobalObject>(&GV)) {
----------------
Not necessarily, at least in this implementation, because "vtable symbol can be
internalized" doesn't imply "all vcalls are visible". See main response.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63932/new/
https://reviews.llvm.org/D63932
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
