evgeny777 added inline comments.
================
Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
c1->f();
- // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+ // ITANIUM: type.test{{.*}}!"_ZTS2C2"
// MS: type.test{{.*}}!"?AUC2@@"
----------------
tejohnson wrote:
> evgeny777 wrote:
> > tejohnson wrote:
> > > evgeny777 wrote:
> > > > What caused this and other changes in this file?
> > > Because we now will insert type tests for non-hidden vtables. This is
> > > enabled by the changes to LTO to interpret these based on the
> > > vcall_visibility metadata.
> > The results of this test case
> > ```
> > %clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 -fms-extensions
> > -fwhole-program-vtables -flto-visibility-public-std -emit-llvm -o - %s |
> > FileCheck --check-prefix=MS --check-prefix=MS-NOSTD %s
> > ```
> > look not correct to me. I think you shouldn't generate type tests for
> > standard library classes with `-flto-visibility-public-std`. Currently if
> > this flag is given, clang doesn't do this either even with
> > `-fvisibility=hidden`
> The associated vtables would get the vcall_visibility public metadata, so the
> type tests themselves aren't problematic. I tend to think that an application
> using such options should simply stick with -fvisibility=hidden to get WPD
> and not use the new LTO option to convert all public vcall_visibility
> metadata to hidden.
> The associated vtables would get the vcall_visibility public metadata, so the
> type tests themselves aren't problematic. I tend to think that an application
> using such options should simply stick with -fvisibility=hidden to get WPD
> and not use the new LTO option to convert all public vcall_visibility
> metadata to hidden.
I see two issues here:
1) It's not always good option to force hidden visibility for everything. For
instance I work on proprietary platform which demands public visibility for
certain symbols in order for dynamic loader to work properly. In this context
your patch does a great job.
2) Standard library is almost never LTOed so in general we can't narrow std::*
vtables visibility to linkage unit
Is there anything which prevents from implementing the same functionality with
new -lto-whole-program-visibility option (i.e without forcing hidden
visibility)? In other words the following looks good to me:
```
# Compile with lto/devirtualization support
clang -flto=thin -flto-visibility-public-std -fwhole-program-vtables -c *.cpp
# Link: everything is devirtualized except standard library classes virtual
methods
clang -Wl,-lto-whole-program-visibility -fuse-ld=lld *.o
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71913/new/
https://reviews.llvm.org/D71913
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
