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

[ben via Phabricator via llvm-commits]

bd1976llvm marked 3 inline comments as done.
bd1976llvm added a comment.

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.



================
Comment at: clang/include/clang/Basic/TargetInfo.h:1096
+  /// Does this target aim for source level compatibility with
+  /// Microsoft C++ code using dllimport/export attributes?
+  virtual bool shouldDLLImportComdatSymbols() const {
----------------
compnerd wrote:
> 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.
Thanks.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:6513
   bool IsInline = false, IsStaticDataMember = false, IsQualifiedFriend = false;
-  bool IsMicrosoft =
-      S.Context.getTargetInfo().getCXXABI().isMicrosoft() ||
-      S.Context.getTargetInfo().getTriple().isWindowsItaniumEnvironment();
+  bool IsMicrosoft = S.Context.getTargetInfo().shouldDLLImportComdatSymbols();
   if (const auto *VD = dyn_cast<VarDecl>(NewDecl)) {
----------------
compnerd wrote:
> I think that renaming `IsMicrosoft` is now warranted.
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.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:9766
         // attribute to a template with a previous instantiation declaration.
         // MinGW doesn't allow this.
         auto *A = cast<InheritableAttr>(
----------------
compnerd wrote:
> 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.
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90299/new/

https://reviews.llvm.org/D90299