[PATCH] D46052: GNUstep Objective-C ABI version 2

[David Chisnall via Phabricator via cfe-commits]

theraven marked 3 inline comments as done.
theraven added inline comments.


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:439
+                             ArrayRef<llvm::Constant *> IvarOffsets,
+                             ArrayRef<llvm::Constant *> IvarAlign,
+                             ArrayRef<Qualifiers::ObjCLifetime> IvarOwnership);
----------------
rjmccall wrote:
> theraven wrote:
> > DHowett-MSFT wrote:
> > > While we're here, is there value in storing the ivar size in layout as well, so that the runtime doesn't need to calculate it from the distance to the next ivar/end of the instance?
> > Normally the runtime calculates it from the type encoding, if it's required.  I agree that it might be nice to have though.  Do you have strong feelings about needing it in the 2.0 ABI?  The looser coupling means that it would be easy to add in the 2.1 ABI if we want it later.  Are you seeing cases where the runtime is calculating it incorrectly because of insufficient information in the type encoding, or where calculating it is causing performance problems?
> The distance between ivar offsets wouldn't be correct anyway because of alignment padding.  A set-ivar function might be able to get away with copying too many bytes from an input buffer (although I wouldn't recommend it!), but a get-ivar function definitely should not copy too many bytes into an output buffer.
> 
> And all that's assuming that the runtime promises not to reorder ivars dynamically.  I don't know what the GNU runtime says about that.  (Static reordering is fine, since the runtime can reasonably demand that the compiler emit ivars in layout order.)
The runtime was calculating the size from the type encoding (which can also be wrong in a few cases, such as packed structures).  I agree it makes sense to add the size though, and have done..


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:1172
+    if (isWeak) {
+      // Placeholder for the real symbol.
+      ClassSymbol->setInitializer(new llvm::GlobalVariable(TheModule,
----------------
rjmccall wrote:
> I would suggest clarifying in what sense this is a placeholder.  Does the runtime recognize it specially?  If so, how?  Is it replaced statically by a later pass in IRGen?
This comment was inherited from the old version and is actually nonsense now.  I have replaced it with one that actually makes sense.


Repository:
  rC Clang

https://reviews.llvm.org/D46052