tejohnson marked an inline comment as done.
tejohnson 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@@"
----------------
pcc wrote:
> tejohnson wrote:
> > evgeny777 wrote:
> > > 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
> > > ```
> > Ok, thanks for the info. I will go ahead and change the code to not insert
> > the type tests in this case to support this usage.
> >
> > Ultimately, I would like to decouple the existence of the type tests from
> > visibility implications. I'm working on another change to delay
> > lowering/removal of type tests until after indirect call promotion, so we
> > can use them in other cases (streamlined indirect call promotion checks
> > against the vtable instead of the function pointers, also useful if we want
> > to implement speculative devirtualization based on WPD info). In those
> > cases we need the type tests, either to locate information in the summary,
> > or to get the address point offset for a vtable address compare. In that
> > case it would be helpful to have the type tests in this type of code as
> > well. So we'll need another way to communicate down to WPD that they should
> > never be devirtualized. But I don't think it makes sense to design this
> > support until there is a concrete use case and need. In the meantime I will
> > change the code to be conservative and not insert the type tests in this
> > case.
> Note that `-flto-visibility-public-std` is a cc1-only option and only used on
> Windows, and further that `-lto-whole-program-visibility` as implemented
> doesn't really make sense on Windows because the classes with public
> visibility are going to be marked dllimport/dllexport/uuid (COM), and
> `-lto-whole-program-visibility` corresponds to flags such as
> `--version-script` or the absence of `-shared` in which the linker
> automatically relaxes the visibility of some symbols, while there is no such
> concept of relaxing symbol visibility on Windows.
>
> I would be inclined to remove this support and either let the public
> visibility automatically derive from the absence of
> `-lto-whole-program-visibility` at link time in COFF links or omit the IR
> needed to support `lto-whole-program-visibility` on Windows.
To clarify, from your first paragraph:
> Note that -flto-visibility-public-std is a cc1-only option and only used on
> Windows, and further that -lto-whole-program-visibility as implemented
> doesn't really make sense on Windows because the classes with public
> visibility are going to be marked dllimport/dllexport/uuid (COM), and
> -lto-whole-program-visibility corresponds to flags such as --version-script
> or the absence of -shared in which the linker automatically relaxes the
> visibility of some symbols, while there is no such concept of relaxing symbol
> visibility on Windows.
Are we currently doing the wrong thing on Windows with
-lto-whole-program-visibility, or are you saying it is ineffective anyway? I am
not very familiar with Windows linking behavior.
> I would be inclined to remove this support
Which support are you referring to here? I initially thought you meant my
change discussed above to skip adding the type tests in the
-flto-visibility-public-std case, but reading the rest of the sentence below I
am not so sure I follow.
> and either let the public visibility automatically derive from the absence of
> -lto-whole-program-visibility at link time in COFF links or omit the IR
> needed to support lto-whole-program-visibility on Windows.
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
