smeenai added a subscriber: rnk.
smeenai added a comment.

Sorry for the belated response; I was on vacation.

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 also don't see any tests added for the codegen changes around `@finally`. I'm 
planning on changing that approach anyway (by pushing 
https://reviews.llvm.org/D47988) through, but it still seems like something 
that was missing from this patch.



================
Comment at: lib/AST/MicrosoftMangle.cpp:1886
   case BuiltinType::ObjCId:
-    mangleArtificalTagType(TTK_Struct, "objc_object");
+    mangleArtificalTagType(TTK_Struct, ".objc_object");
     break;
----------------
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.


================
Comment at: lib/CodeGen/CGObjCRuntime.cpp:203
+        const Stmt *FinallyBlock = Finally->getFinallyBody();
+        HelperCGF.startOutlinedSEHHelper(CGF, /*isFilter*/false, FinallyBlock);
+
----------------
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).


Repository:
  rC Clang

https://reviews.llvm.org/D50144



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to