tejohnson marked an inline comment as done.
tejohnson added inline comments.
Herald added a reviewer: MaskRay.
================
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:
> pcc wrote:
> > tejohnson wrote:
> > > 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.
> > > 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'm saying that it is ineffective. Windows linkers don't have
> > `-lto-whole-program-visibility`, so either way the class would end up with
> > public LTO visibility.
> >
> > > 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
> >
> > Yes, that is what I meant. Sorry if I wasn't clear. That would give us the
> > behaviour of the first option that I mentioned, which would be a
> > consequence of treating classes in `std` the same way as other classes. My
> > second option would be to do that as well as making the compiler omit the
> > type tests for all public LTO visibility classes when targeting Windows
> > (not just the classes in `std` when `-flto-visibility-public-std` is
> > passed), but that's more of an optimization.
> Ok, I am inclined to simply remove this special handling then, which goes
> back to the original intent which is that type tests don't imply visibility.
> This allows their use in follow on cases such as guiding ICP with whole
> program class hierarchy or type test info. I'll send a patch as soon as I get
> a chance.
> Ok, I am inclined to simply remove this special handling then, which goes
> back to the original intent which is that type tests don't imply visibility.
> This allows their use in follow on cases such as guiding ICP with whole
> program class hierarchy or type test info. I'll send a patch as soon as I get
> a chance.
Sorry, it took a lot longer to come back and make this change than I intended.
Fix mailed in D83845.
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
