[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/CodeGen/CGObjCGNU.cpp:3542
+      allSelectors.push_back(entry.first);
+    std::sort(allSelectors.begin(), allSelectors.end());
 
----------------
mgrang wrote:
> Please use llvm::sort instead of std::sort. See https://llvm.org/docs/CodingStandards.html#beware-of-non-deterministic-sorting-order-of-equal-elements.
Also, why is the sort necessary?  Should this change be separately committed and tested?


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:915
+    return name;
+  }
   /// The GCC ABI superclass message lookup function.  Takes a pointer to a
----------------
theraven wrote:
> 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
I don't mind if the extracted patch seems unmotivated on its own, but I do think it should be 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);
----------------
theraven wrote:
> 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). 
Okay, the exception logic makes sense.  Please make this a common predicate instead of repeating it in different places.

My understanding is that the restriction on the use of `dllimport`-ed symbols is a general PE restriction: the format itself only supports binds in the import address table, and everything else has just to be a rebase.  I mean, of course you can imagine a loader that extends PE to support arbitrary binds, but you can also imagine a loader that extends PE to support just embedding an ELF image.  Firmware probably never uses imported symbols.


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:3817
+  if (isRethrow && CGF.CGM.getTarget().getTriple().isWindowsMSVCEnvironment()) {
+    CGF.EmitRuntimeCallOrInvoke(ExceptionReThrowFn).setDoesNotReturn();
+  }
----------------
theraven wrote:
> rjmccall wrote:
> > You're sure here that the static information aligns with the dynamic?
> I'm not sure I understand this question.
Your new code path no longer uses `ExceptionAsObject`, which is our static notion of the current exception value.  Instead, you call a runtime function which presumably relies on a dynamically-stored current exception value.  I'm just asking if, in this lexical position, you're definitely rethrowing the right exception.


================
Comment at: lib/CodeGen/CGObjCRuntime.cpp:125
     llvm::Constant *TypeInfo;
+    unsigned Flags;
   };
----------------
theraven wrote:
> 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.
Just mentioning that they're the same catch flags used in the Microsoft C++ ABI is all I'm asking for.  Although maybe there could be an abstraction for that instead of passing them around separately?


Repository:
  rC Clang

https://reviews.llvm.org/D50144