Page MenuHomePhabricator
Differential D71913 Diff 240042 clang/test/CodeGenCXX/lto-visibility-inference.cpp

Changeset View

Standalone View

View Options

clang/test/CodeGenCXX/lto-visibility-inference.cpp

Show First 20 Lines • Show All 64 Lines • ▼ Show 20 Lines
} }
void f(C1 *c1, C2 *c2, C3 *c3, C4 *c4, C5 *c5, C6 *c6, std::C7 *c7, void f(C1 *c1, C2 *c2, C3 *c3, C4 *c4, C5 *c5, C6 *c6, std::C7 *c7,
std::C7::C8 *c8, stdext::C9 *c9, other::C10 *c10) { std::C7::C8 *c8, stdext::C9 *c9, other::C10 *c10) {
// ITANIUM: type.test{{.*}}!"_ZTS2C1" // ITANIUM: type.test{{.*}}!"_ZTS2C1"
// MS: type.test{{.*}}!"?AUC1@@" // MS: type.test{{.*}}!"?AUC1@@"
c1->f(); c1->f();
// ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2" // ITANIUM: type.test{{.*}}!"_ZTS2C2"
evgeny777Unsubmitted
Not Done
ReplyInline Actions

What caused this and other changes in this file?

evgeny777: What caused this and other changes in this file?
tejohnsonAuthorUnsubmitted
Done
ReplyInline Actions

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.

tejohnson: Because we now will insert type tests for non-hidden vtables. This is enabled by the changes to…
evgeny777Unsubmitted
Not Done
ReplyInline Actions

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

evgeny777: The results of this test case ``` %clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11…
tejohnsonAuthorUnsubmitted
Done
ReplyInline Actions

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.

tejohnson: The associated vtables would get the vcall_visibility public metadata, so the type tests…
evgeny777Unsubmitted
Not Done
ReplyInline Actions

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.
  1. 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
evgeny777: > The associated vtables would get the vcall_visibility public metadata, so the type tests…
tejohnsonAuthorUnsubmitted
Done
ReplyInline Actions

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.

tejohnson: Ok, thanks for the info. I will go ahead and change the code to not insert the type tests in…
pccUnsubmitted
Not Done
ReplyInline Actions

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.

pcc: Note that `-flto-visibility-public-std` is a cc1-only option and only used on Windows, and…
tejohnsonAuthorUnsubmitted
Done
ReplyInline Actions

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.

tejohnson: To clarify, from your first paragraph: > Note that -flto-visibility-public-std is a cc1-only…
pccUnsubmitted
Not Done
ReplyInline Actions

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.

pcc: > Are we currently doing the wrong thing on Windows with -lto-whole-program-visibility, or are…
tejohnsonAuthorUnsubmitted
Done
ReplyInline Actions

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.

tejohnson: Ok, I am inclined to simply remove this special handling then, which goes back to the original…
tejohnsonAuthorUnsubmitted
Done
ReplyInline Actions

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.

tejohnson: > Ok, I am inclined to simply remove this special handling then, which goes back to the…
// MS: type.test{{.*}}!"?AUC2@@" // MS: type.test{{.*}}!"?AUC2@@"
c2->f(); c2->f();
// ITANIUM: type.test{{.*}}!"_ZTS2C3" // ITANIUM: type.test{{.*}}!"_ZTS2C3"
// MS-NOT: type.test{{.*}}!"?AUC3@@" // MS: type.test{{.*}}!"?AUC3@@"
c3->f(); c3->f();
// ITANIUM: type.test{{.*}}!"_ZTS2C4" // ITANIUM: type.test{{.*}}!"_ZTS2C4"
// MS-NOT: type.test{{.*}}!"?AUC4@@" // MS: type.test{{.*}}!"?AUC4@@"
c4->f(); c4->f();
// ITANIUM-NOT: type.test{{.*}}!"_ZTS2C5" // ITANIUM: type.test{{.*}}!"_ZTS2C5"
// MS-NOT: type.test{{.*}}!"?AUC5@@" // MS: type.test{{.*}}!"?AUC5@@"
c5->f(); c5->f();
// ITANIUM-NOT: type.test{{.*}}!"_ZTS2C6" // ITANIUM: type.test{{.*}}!"_ZTS2C6"
// MS-NOT: type.test{{.*}}!"?AUC6@@" // MS: type.test{{.*}}!"?AUC6@@"
c6->f(); c6->f();
// ITANIUM: type.test{{.*}}!"_ZTSSt2C7" // ITANIUM: type.test{{.*}}!"_ZTSSt2C7"
// MS-STD: type.test{{.*}}!"?AUC7@std@@" // MS-STD: type.test{{.*}}!"?AUC7@std@@"
// MS-NOSTD-NOT: type.test{{.*}}!"?AUC7@std@@" // MS-NOSTD-NOT: type.test{{.*}}!"?AUC7@std@@"
c7->f(); c7->f();
// ITANIUM: type.test{{.*}}!"_ZTSNSt2C72C8E" // ITANIUM: type.test{{.*}}!"_ZTSNSt2C72C8E"
// MS-STD: type.test{{.*}}!"?AUC8@C7@std@@" // MS-STD: type.test{{.*}}!"?AUC8@C7@std@@"
// MS-NOSTD-NOT: type.test{{.*}}!"?AUC8@C7@std@@" // MS-NOSTD-NOT: type.test{{.*}}!"?AUC8@C7@std@@"
Show All 13 Lines