[PATCH] D50144: Add Windows support for the GNUstep Objective-C ABI V2.

[David Chisnall via Phabricator via cfe-commits]

theraven added inline comments.


================
Comment at: lib/CodeGen/CGBlocks.cpp:1262
+  if (IsWindows) {
+    auto *Init = llvm::Function::Create(llvm::FunctionType::get(CGM.VoidTy,
+          {}), llvm::GlobalValue::InternalLinkage, ".block_isa_init",
----------------
rjmccall wrote:
> theraven wrote:
> > DHowett-MSFT wrote:
> > > Is there value in emitting a list of blocks that need to be initialized, then initializing them in one go in a COMDAT-foldable function?
> > I think that the best solution is to move this into the back end, so that this code goes away in the front end, but anything that's referring to a dllimport global in a global initialiser is transformed in the back end to a list of initialisations and a comdat function that walks the list and sets them up.  That said, this seems sufficiently generally useful that it would be nice for the function to be in the CRT bits.  
> > 
> > 
> > I should be in Redmond some time in October, so maybe we can discuss it with some of the VS team then?
> Can the blocks part of this patch be split out?  It's basically a totally different bugfix.
Split into D50436.


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:915
+    return name;
+  }
   /// The GCC ABI superclass message lookup function.  Takes a pointer to a
----------------
rjmccall wrote:
> Can this section-names cleanup also just be in a separate patch?
This is difficult to extract, because it's mostly needed for the COFF part where we need to modify the section names.  For ELF, it was fine to keep them as separate `const char*`s


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:2262
 llvm::Constant *CGObjCGNUstep::GetEHType(QualType T) {
+  if (CGM.getContext().getTargetInfo().getTriple().isWindowsMSVCEnvironment())
+    return CGM.getCXXABI().getAddrOfRTTIDescriptor(T);
----------------
rjmccall wrote:
> I think this is the third different way that you test for Windows in this patch. :) Is there a reason why this is just testing for MSVC and the others are testing for PE/COFF or for Windows generally?
For the EH logic, we are using DWARF exceptions if we are targeting a Windows environment that uses DWARF EH and SEH-based exceptions if we are targeting a Windows environment that uses MSVC-compatible SEH.

The section code is specific to PE/COFF, where we don't get to use some of the ELF tricks.

The blocks code is specific to Windows, because it relates to Windows run-time linking.  Hypothetically, a PE/COFF platform could provide dynamic relocations that would eliminate the need for that check.

It's possible that some of the PE/COFF vs Windows distinctions are wrong.  For example, UEFI images are PE/COFF binaries and if anyone wanted to use blocks in a UEFI firmware image then they may find that they need the Windows code path (but I do not have a good way of testing this, so restricted it to Windows initially).  Similarly, some of the section initialisation code in CGObjCGNU may actually be Windows-only, but it's probably a good starting point for anyone who actually wants to use Objective-C in UEFI firmware (though the final destination for such people is likely to involve padded cells). 


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:3817
+  if (isRethrow && CGF.CGM.getTarget().getTriple().isWindowsMSVCEnvironment()) {
+    CGF.EmitRuntimeCallOrInvoke(ExceptionReThrowFn).setDoesNotReturn();
+  }
----------------
rjmccall wrote:
> You're sure here that the static information aligns with the dynamic?
I'm not sure I understand this question.


================
Comment at: lib/CodeGen/CGObjCRuntime.cpp:125
     llvm::Constant *TypeInfo;
+    unsigned Flags;
   };
----------------
rjmccall wrote:
> Please add some sort of comment about the meaning and source of these flags.
These are not well documented anywhere, including in the Clang Microsoft C++ ABI code that I read to see why exceptions weren't working, but I've added a small comment that explains why they exist.  I have no idea where the values come from though.


Repository:
  rC Clang

https://reviews.llvm.org/D50144