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

Thu Aug 23 03:56:15 PDT 2018

theraven added a comment.

In https://reviews.llvm.org/D50144#1209758, @smeenai wrote:

> I'm confused by the funclet-based unwinding support. Do you intend GNUStep to be used with windows-msvc triples? If so, how is the runtime throwing exceptions? We took the approach of having the Obj-C runtime wrap Obj-C exceptions in C++ exceptions and then have vcruntime handle the rest (see https://reviews.llvm.org/D47233 and https://reviews.llvm.org/D47988), but I don't see any of the associated RTTI emission changes in this patch.

I'm assuming 'we' here is Apple?  Microsoft has a friendly fork of the GNUstep runtime in the WinObjC project and I am slowly working on pulling in the WinObjC downstream changes, incorporating them with the v2 ABI, and making it easy for WinObjC to use an unmodified version of the runtime.  The v2 ABI still has a few things that don't work with the Windows linkage model, which I'm working out how best to address.  The code for throwing an exception is here:

https://github.com/gnustep/libobjc2/blob/master/eh_win32_msvc.cc

The runtime constructs an SEH exception on the stack.  The low-level NT exception type knows all of the possible types that might be allowed in a catch, so the runtime dynamically constructs a list of all of these, one per superclass in the hierarchy, on the stack.  The funclet-based unwinding mechanism makes it safe to do this, because the throwing stack frame remains valid until after the exception is caught.  The Windows C++ personality function then checks the catches the exceptions.  Clang/CX (Clang with the Visual Studio compiler back end) has supported this for a while, but it never made it upstream to Clang/LLVM and I'm trying to rectify this.

On 64-bit Windows, we must provide 32-bit offsets to some arbitrary base.  For C++, this is the base load address of the DLL.  That's fine for C++, where the throw types are static (and the entire class hierarchy is available at compile time), but our stack may be more than 4GB away from any library.  Instead, we use an offset from some arbitrary on-stack location (and if we end up being more than 4GB away from the base of our stack frame, then something's gone very badly wrong!).

================
Comment at: lib/AST/MicrosoftMangle.cpp:1886
   case BuiltinType::ObjCId:
-    mangleArtificalTagType(TTK_Struct, "objc_object");
+    mangleArtificalTagType(TTK_Struct, ".objc_object");
     break;
----------------
smeenai wrote:
> theraven wrote:
> > smeenai wrote:
> > > theraven wrote:
> > > > compnerd wrote:
> > > > > DHowett-MSFT wrote:
> > > > > > I'm interested in @smeenai's take on this, as well.
> > > > > Hmm, doesn't this break ObjC/ObjC++ interoperability?  I don't think that we should be changing the decoration here for the MS ABI case.
> > > > No, this fixes ObjC++ interop.  Throwing an exception from a `@throw` and catching it with `catch` now works and we avoid the problem that a C++ compilation unit declares a `struct` that happens to have the same name as an Objective-C class (which is very common for wrapper code) may catch things of the wrong type.
> > > I've seen a lot of code which does something like this in a header
> > > 
> > > ```lang=cpp
> > > #ifdef __OBJC__
> > > @class I;
> > > #else
> > > struct I;
> > > #endif
> > > 
> > > void f(I *);
> > > ```
> > > 
> > > You want `f` to be mangled consistently across C++ and Objective-C++, otherwise you'll get link errors if `f` is e.g. defined in a C++ TU and referenced in an Objective-C++ TU. This was the case before your change and isn't the case after your change, which is what @compnerd was pointing out. They are mangled consistently in the Itanium ABI, and we want them to be mangled consistently in the MS ABI as well.
> > > 
> > > Not wanting the catch mix-up is completely reasonable, of course, but it can be done in a more targeted way. I'll put up a patch that undoes this part of the change while still preventing incorrect catching.
> > With a non-fragile ABI, there is not guarantee that `struct I` and `@class I` have the same layout.  This is why `@defs` has gone away.  Any code that does this and treats `struct I` as anything other than an opaque type is broken.  No other platform supports throwing an Objective-C object and catching it as a struct, and this is an intentional design decision.  I would be strongly opposed to a change that intentionally supported this, because it is breaking correct code in order to make incorrect code do something that may or may not work.
> To be clear, I'm aware of the layout differences. My plan was to revert the mangling here to what it was before (which would also be consistent with Itanium), and then adjust the RTTI emission for Obj-C exception types (which I'm handling in https://reviews.llvm.org/D47233) to differentiate between C++ and Obj-C exceptions, which would prevent unintended catches.
Okay, that sounds fine.  The runtime needs to know about the mangling to generate the types, but we haven't yet done the 2.0 release of the runtime and Windows support is incomplete with the new ABI in the clang 7 release branch, so will be marked as an experimental feature.  We've merged something based on the WinObjC changes, though with some clean-ups so that it works correctly on Win64, but can change the mangling up until we've made the 2.0 release.

================
Comment at: lib/CodeGen/CGObjCRuntime.cpp:203
+        const Stmt *FinallyBlock = Finally->getFinallyBody();
+        HelperCGF.startOutlinedSEHHelper(CGF, /*isFilter*/false, FinallyBlock);
+
----------------
smeenai wrote:
> This is *very* limiting. This outlining doesn't handle capturing self, capturing block variables, and lots of other cases that come up in practice. I attempted to implement some of the missing cases, before meeting with @compnerd and @rnk and deciding that it was better to use CapturedStmt instead. See https://reviews.llvm.org/D47564 for more discussion, and https://reviews.llvm.org/D47988 implements the associated codegen (unfortunately I wasn't able to get that through before going on vacation).
This worked for the cases that I tested, but that was far from exhaustive.  I'm very happy to switch to something better, and D47988 does, indeed, look like something better.

Repository:
  rC Clang

https://reviews.llvm.org/D50144