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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/AST/MicrosoftMangle.cpp:448
     mangleVariableEncoding(VD);
-  else
+  else if (!isa<ObjCInterfaceDecl>(D))
     llvm_unreachable("Tried to mangle unexpected NamedDecl!");
----------------
I don't think we want `ObjCInterfaceDecl`s to enter this function normally; I'd prefer to leave that assertion intact.


================
Comment at: lib/AST/MicrosoftMangle.cpp:2574
   mangleTagTypeKind(TTK_Struct);
-  mangleName(T->getDecl());
+  mangle(T->getDecl(), ".objc_cls_");
 }
----------------
You're not really reusing any interesting logic from `mangle` here; please just stream the literal directly into `Out`.

Also, this will add the actual identifier to the substitution set, whereas your implementation below in the case for ObjCObjectType adds the prefixed identifier to the substitution set.


================
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",
----------------
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.


================
Comment at: lib/CodeGen/CGBlocks.cpp:1216
   bool IsOpenCL = CGM.getLangOpts().OpenCL;
+  bool IsWindows = CGM.getTarget().getTriple().isOSWindows();
   if (!IsOpenCL) {
----------------
Should this be a PE/COFF-specific restriction?  Or otherwise somehow restricted to the "native" environment?  I don't know enough about all the UNIX-on-Windows approaches to know whether the same restriction would apply to all of them.  I did think they generally used the Itanium C++ ABI, and the Itanium ABI relies on linking v-tables from the C++ standard library.  Maybe those environments just all use static linking to get around that.


================
Comment at: lib/CodeGen/CGBlocks.cpp:1276
+    InitVar->setSection(".CRT$XCLa");
+    CGM.addUsedGlobal(InitVar);
+  }
----------------
Is the priority system not good enough?


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:915
+    return name;
+  }
   /// The GCC ABI superclass message lookup function.  Takes a pointer to a
----------------
Can this section-names cleanup also just be in a separate patch?


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:2262
 llvm::Constant *CGObjCGNUstep::GetEHType(QualType T) {
+  if (CGM.getContext().getTargetInfo().getTriple().isWindowsMSVCEnvironment())
+    return CGM.getCXXABI().getAddrOfRTTIDescriptor(T);
----------------
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?


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


================
Comment at: lib/CodeGen/CGObjCRuntime.cpp:125
     llvm::Constant *TypeInfo;
+    unsigned Flags;
   };
----------------
Please add some sort of comment about the meaning and source of these flags.


================
Comment at: lib/CodeGen/EHScopeStack.h:424
+	void Emit(CodeGenFunction &CGF, Flags F) override;
+};
+
----------------
Please add a function on SGF to push this cleanup or something instead of globally declaring it.


Repository:
  rC Clang

https://reviews.llvm.org/D50144