This is an archive of the discontinued LLVM Phabricator instance.

[PS4] handle dllimport/export w.r.t vtables/rtti
ClosedPublic

Authored by bd1976llvm on Dec 14 2020, 3:59 AM.

Details

Summary

The existing Windows Itanium patches for dllimport/export behaviour w.r.t vtables/rtti can't be adopted for PS4 due to backwards compatibility reasons (see comments on https://reviews.llvm.org/D90299).

This review is for adding our PS4 scheme for this to Clang.

Diff Detail

Event Timeline

bd1976llvm requested review of this revision.Dec 14 2020, 3:59 AM
bd1976llvm created this revision.
compnerd added inline comments.Dec 14 2020, 9:27 AM
clang/include/clang/Basic/TargetInfo.h
1134

Please reflow the comment (or clang-format)

clang/lib/CodeGen/ItaniumCXXABI.cpp
1843

Can this not be const auto *D : RD->noload_decls()?

1844–1850

I think that the additional conditional and early break are a bit easier to read.

1924

I'm having a hard time following this comment.

I don't think that "Within the context of the Itanium C++ ABI, we want to mimic this MS behavior." adds any value.

What exactly does "defined in this TU/externally" mean? TU or defined externally means everywhere, no?

What is the "MS constraint"?

3184

Please reflow the text (or clang-format)

3829

I'm confused - does PS4 use a custom scheme or the MS ABI rules?

bd1976llvm added inline comments.Dec 14 2020, 6:15 PM
clang/lib/CodeGen/ItaniumCXXABI.cpp
1924

I will reword.

What is the "MS constraint"?

This is the constraint from the first sentence: "when dllimport/exporting only certain members of a class (rather than the non-inline virtual methods must be dllimported/exported, otherwise a link error will result". This constraint is documented here: https://docs.microsoft.com/en-us/cpp/cpp/using-dllimport-and-dllexport-in-cpp-classes?view=msvc-160#_pluslang_using_dllimport_and_dllexport_in_c2b2bselectivememberimportexport as "If you have a class in which you are using selective member import/export with virtual functions, the functions must be in the exportable interface or defined inline (visible to the client).".

What exactly does "defined in this TU/externally" mean? TU or defined externally means everywhere, no?

Sorry badly phrased, try: "For classes using selective member import/export: We dllimport the vtable if it is defined externally and all the non-inline virtual methods are marked dllimport. We dllexport the vtable if it is defined in this TU and all the non-inline virtual methods are marked dllexport. Otherwise the vtable is not imported or exported. With this scheme links succeed in circumstances where they would succeed for Microsoft toolchains and fail with a linker error (missing vtable for struct) in circumstances where MS toolchains would give a linker error.

Hope that makes it clearer. There is a wordier comment in the attached test explaining this.

3829

Sorry. Confusingly written :( The "custom scheme" I'm referring to is the patches in this review. The goal is to match the MS toolchain behaviour for dllimport/export on classes with vtables. This comment should probably just state: " We export the typeinfo in the same circumstances as the vtable is exported.".

bd1976llvm marked 6 inline comments as done.

Thanks for the suggestions. I have reworded the comments so that they are hopefully much clearer.

I have added this patch to https://reviews.llvm.org/D88124 along with some end to end tests to show the scei vendor windows-itanium behaviour. From the 'wi-test/test/dll_vtable_missing/test.py' test you can see that the link error given by the scei vendor variant is more consistent than the standard windows-itanium. Both variants should be equivalent in terms of runtime behaviour.

bd1976llvm edited the summary of this revision. (Show Details)Mar 12 2021, 3:04 AM

@compnerd, @smeenai do you have any further comments?

FTR, this looks okay from the PS4 owner's perspective, although IIRC @wristow did our original local patches.

@compnerd it looks like @bd1976bris has addressed all the comments; is there anything blocking an LGTM here?

@compnerd, @smeenai do you have any further comments?

Sorry for the lack of review on my part here; I haven't had any time to look at this in detail.

wristow accepted this revision.Mar 31 2021, 11:58 AM

LGTM!

This revision is now accepted and ready to land.Mar 31 2021, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2021, 3:41 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript