This is an archive of the discontinued LLVM Phabricator instance.

[windows-itanium] handle dllimport/export code paths separately and share with PS4
ClosedPublic

Authored by bd1976llvm on Oct 28 2020, 4:46 AM.

Details

Summary

Similar to Windows Itanium, PS4 is also an Itanium C++ ABI variant and shares the goal of source code compatibility with Microsoft code that uses dllimport/export.

This change introduces a new function to determine from the triple if an environment aims for compatibility with MS w.r.t these attributes and guards the relevant code paths using that function.

Diff Detail

Event Timeline

bd1976llvm created this revision.Oct 28 2020, 4:46 AM
Herald added a project: Restricted Project. · View Herald Transcript
bd1976llvm requested review of this revision.Oct 28 2020, 4:46 AM
compnerd accepted this revision.Oct 29 2020, 8:50 AM
compnerd added subscribers: mstorsjo, rnk.

Thanks, this is definitely a nice cleanup. I would say isDLLStorageEnvironment is a better predicate. Would be nice to get @rnk's opinion (and @mstorsjo, if they are interested).

This revision is now accepted and ready to land.Oct 29 2020, 8:50 AM
rnk added a comment.Oct 29 2020, 12:01 PM

Will there be any need to check this in LLVM? If not, I would prefer to put this on clang::TargetInfo.

clang/lib/CodeGen/ItaniumCXXABI.cpp
3201 ↗(On Diff #301237)

CGM also has a getTarget() method to get TargetInfo if the predicate moves there.

I would say isDLLStorageEnvironment is a better predicate.

That sounds less specific to me, and would imply that it also would cover mingw targets (which it doesn't).

The current suggested name, isDLLImportExportMSCompatible, is more specific, but when I first read it my reaction was that mingw's dllexport/import (in the general cases) for sure works the same as for MSVC - if you consider C functions. So if the name can make it clearer that this covers C++ cases, it'd be more intuitive to me, even though I'm sure it'd become even more of a mouthful then...

rnk added a comment.Oct 29 2020, 1:14 PM

I agree, the name could be improved. For prior art, see /Zc:dllexportInlines-, which disables some (but not all) of the dllexport/import semantics in clang-cl.

What we are really talking about here is what should be done when importing or exporting C++ constructs with vague linkage (inline functions, templates). That's what all the affected code has in common. Clang doesn't often use the terminology of vague linkage, so maybe it would be better to use the name comdat somehow.

How about TargetInfo::shouldDLLImportComdatSymbols? You could add in "export" (shouldDLLImportExportComdatSymbols), but GCC also exports comdat things sometimes, and in the cases where this predicate controls export behavior, it's pretty clear that it needs to be done to support the side which would import the symbol (explicit template instantiations).

In D90299#2362895, @rnk wrote:

I agree, the name could be improved. For prior art, see /Zc:dllexportInlines-, which disables some (but not all) of the dllexport/import semantics in clang-cl.

What we are really talking about here is what should be done when importing or exporting C++ constructs with vague linkage (inline functions, templates). That's what all the affected code has in common. Clang doesn't often use the terminology of vague linkage, so maybe it would be better to use the name comdat somehow.

How about TargetInfo::shouldDLLImportComdatSymbols? You could add in "export" (shouldDLLImportExportComdatSymbols), but GCC also exports comdat things sometimes, and in the cases where this predicate controls export behavior, it's pretty clear that it needs to be done to support the side which would import the symbol (explicit template instantiations).

Thanks for the comments. Naming is always hard! I will adopt @rnk's shouldDLLImportComdatSymbols unless anyone has an objection?

bd1976llvm updated this revision to Diff 302428.Nov 2 2020, 4:14 PM

Addressed review comments.

bd1976llvm marked an inline comment as done.Nov 2 2020, 4:15 PM
bd1976llvm added inline comments.
clang/lib/CodeGen/ItaniumCXXABI.cpp
3201 ↗(On Diff #301237)

Thanks!

rnk added a comment.Nov 2 2020, 4:35 PM

Please add tests for the common cases:

  • exported class with inline methods
  • exported explicit class template instantiation
  • imported class with inline methods
  • imported extern class template declaration

You might be able to piggyback on any windows-itanium dllimport tests, if they exist, since the LLVM IR should match.

bd1976llvm updated this revision to Diff 303285.Nov 5 2020, 4:12 PM
bd1976llvm marked an inline comment as done.
bd1976llvm edited the summary of this revision. (Show Details)

Add tests and prevent SemaTemplate and ItaniumCXXABI code paths from executing for PS4 (for now).

I have had to be a bit more cautious as to which code paths I enable for PS4 (vs the initial diff) as I realised that we need to do more testing downstream to be sure the patches in SemaTemplate and ItaniumCXXABI will work with the propriety schemes that we are currently using. For the SemaTemplate patches I have added an exception for PS4 (that I expect to remove once we have completed further testing) and tests to show that PS4 doesn't follow Windows Itanium here. For the ItaniumCXXABI patches, I have left the patches as specific to Windows Itanium. Those patches handle how dllimport/export interacts with RTTI data; on PS4 we currently have a custom scheme for this. I am hopeful that we can use those patches in the future and I am working on verifying this internally but there is some chance that we might have to use a PS4 specific scheme for this. I also discovered that RTTI dllexport/import tests currently assert with the upstream PS4 target - so I couldn't write tests to show that PS4 doesn't follow the Windows Itanium behaviour here.

There were existing LIT tests covering all of the code paths. I have added a run line with the PS4 triple to the appropriate ones.

The patches in SemaDecl.cpp, SemaDeclAttr.cpp and SemaDeclCXX.cpp are covered by the tests:

  • Sema/dllimport.c
  • SemaCXX/dllexport.cpp
  • SemaCXX/dllimport.cpp
  • CodeGenCXX/windows-itanium-dllexport.cpp

The patches in SemaTemplate.cpp are covered by:

  • CodeGenCXX/windows-implicit-dllexport-template-specialization.cpp
  • CodeGenCXX/windows-itanium-dllexport.cpp

The code changes in ItaniumCXXABI.cpp are covered by:

  • CodeGenCXX/windows-itanium-type-info.cpp
  • CodeGenCXXwindows-itanium-exceptions.cpp
bd1976llvm requested review of this revision.Nov 13 2020, 3:19 AM

I think this needs a re-review as I have deviated from the original simple approach.

Let me know if you think that more tests need adding. I have added support for LIT based run tests to the build recipe (https://reviews.llvm.org/D88124) I can use this to add any run tests that might be useful for this review.

compnerd added inline comments.Nov 23 2020, 9:17 AM
clang/include/clang/Basic/TargetInfo.h
1096

I don't think its source compatibility, but rather semantic compatibility. -fdeclspec gives you the source level compatibility - it can process the source, just may not do the same thing.

clang/lib/Sema/SemaDecl.cpp
6513

I think that renaming IsMicrosoft is now warranted.

clang/lib/Sema/SemaTemplate.cpp
9766

Please update the comment (I think we can entirely remove it now given the new name). This probably fits better in the definition of shouldImportDLLImportComdatSymbols now as the condition is completely different now.

bd1976llvm marked 3 inline comments as done.Nov 24 2020, 4:56 PM

Thanks for the comments. I have updated the diff.

Just to explain a bit more about why I haven't been able to adopt all the Windows Itanium code paths for PS4: I am confident that the Windows Itanium code is correct for PS4 and it passes all of our downstream testing. However, there is a very strict backwards compatibility requirement that is preventing me from using all of these code paths for PS4 (see https://www.youtube.com/watch?v=ATOfCfM9hSg for a talk about ABI compatibility difficulties). For the SemaTemplate paths I am still hopeful that we can adopt this behaviour. For the ItaniumCXXABI paths I think we can eventually adopt these in the future but for the next few years we will need to use a PS4 specific scheme for handling RTTI and vtables with dllimport/export. I will put a WIP review shortly for this scheme.

clang/include/clang/Basic/TargetInfo.h
1096

Thanks.

clang/lib/Sema/SemaDecl.cpp
6513

What would be better though? The MS ABI is the standard for the dllimport/export behaviour. Therefore, I think IsMicrosoft is quite a good name and it fits well with the existing comments.

clang/lib/Sema/SemaTemplate.cpp
9766

The comment seems helpful to me though, no? Also, this comment is rather tied to the matching comment on L9782. Since the MS ABI is standard for dllimport/export behaviour and MinGW is the outlier (in that it does it's own thing, a bit, as far as C++ dllimport/export goes) it seems reasonable that these two are mentioned?

bd1976llvm marked 3 inline comments as done.

Updated comment to state that "semantic compatibility" is the goal.

compnerd accepted this revision.Nov 25 2020, 8:43 AM
compnerd added inline comments.
clang/lib/Sema/SemaDecl.cpp
6513

Then IsMicrosoftABI sounds better :). The problem with the current name is that its confusing between the Microsoft environment and the Microsoft ABI.

clang/lib/Sema/SemaTemplate.cpp
9766

I think I would be okay with retaining the comment with the initial clause In the MS ABI, removed.

This revision is now accepted and ready to land.Nov 25 2020, 8:43 AM

Addressed review comments.

bd1976llvm marked 2 inline comments as done.Nov 26 2020, 3:59 AM
bd1976llvm added inline comments.
clang/lib/Sema/SemaDecl.cpp
6513

Done. Thanks.

clang/lib/Sema/SemaTemplate.cpp
9766

Done. Thanks!

Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2020, 6:42 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
bd1976llvm marked 2 inline comments as done.Feb 17 2021, 5:56 AM
This comment was removed by bd1976llvm.