GNUstep Objective-C ABI version 2

[theraven]

Summary

This includes initial support for the (hopefully final) updated Objective-C ABI, developed here:

https://github.com/davidchisnall/clang-gnustep-abi-2

It also includes some cleanups and refactoring from older GNU ABIs.

This is not yet in a state that's ready to commit (for example, it's currently ELF-only, but doesn't include driver changes to downgrade to the 1.x ABI for non-ELF targets until other binary formats are supported), but it's probably in a state where it could benefit from review.

[rjmccall]

Are you asking for a code review or a design review of the ABI? The second would be much easier to do from a design document.

lib/CodeGen/CGObjCGNU.cpp

502

There's an isOptional() method now.

[theraven]

In D46052#1078597, @rjmccall wrote:

Are you asking for a code review or a design review of the ABI?

I don't think a design review is appropriate here, I am asking for a code review.

The second would be much easier to do from a design document.

If you'd be willing to do a design review, I'd be very happy to send you a copy of the ABI document.

[theraven]

• Fixed non-use of isOptional (in all three places where it should have been used)
• Reject -fobjc-runtime=gnustep-2.x in the driver for non-ELF targets
• Fix a bug where the wrong variable was assigned in protocol generation, which triggered an assertion failure in one of the runtime's tests.
• Rename protocol reference variables to prevent accidental conflicts with protocol references (this was already done for other indirection variables, but I'd missed protocols)

[theraven]

Fix the symbol names that I thought I'd done previously.

Also a small refactoring of how class init variables are generated to do it at the sensible time.

This version has been tested with the runtime's test suite and the GNUstep-Base (Foundation framework) test suite.

[rjmccall]

lib/CodeGen/CGObjCGNU.cpp

961

I'd encourage you to use ConstantBuilder whenever you would want to use this.

966

Why copy the string? Also, you don't actually use it until later.

977

Please hoist SL->getLength() into a variable.

Also, consider breaking this bit of code (maybe just building a uint64_t?) into a separate function.

992

It's weird to give a struct like this and have it only be right on a *big*-endian target. You might also consider just using uint64_t.

996

I think this might be clearer as 64 - 7 - (i*7).

1039

This is a very misleading variable name.

1040

You should leave a comment saying that it's safe to expand an N-code-unit UTF-8 string into an N-code-unit UTF-16 buffer because UTF-16 never uses more code units than UTF-8.

1077

Is @ really the only illegal character in ELF symbol names? Surely at least \0 as well. And your mangling will collide strings if they contain \1.

I think you should probably just have this use internal linkage (and a meaningless symbol name) for strings containing bad characters.

1081

Surely this has to be linkonce_odr?

1097

Field declaration comments, like you use elsewhere, would help here. And something that includes the struct name that you're generating as well.

1126

Same comment here about adding a comment somewhere that mentions the struct name from the runtime header that you're generating.

1196

Please make this exhaustive, since you're just missing Qualifiers::OCL_Autoreleasing (which is illegal on an ivar, of course).

1216

Why 4?

1395

Maybe you can have a method that returns the start/end symbols as a pair? It would cut down on the redundancy here a lot.

[theraven]

lib/CodeGen/CGObjCGNU.cpp

977

I don't think separating it would simplify the code much. It's 8 lines of non-comment code and you'd probably need at least three of them in the caller.

1077

No, I copied the mangling for selectors without engaging my brain. I've now replaced it with something much more conservative. I might tweak it a bit before the next release, but this seems to give pretty good coverage.

[DHowett-MSFT]

lib/CodeGen/CGObjCGNU.cpp

439

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?

1402

This should be readily expandable for PE/COFF, but we'll need to change some of the section names to fit better. Is it worth abstracting the names of the sections across the target format somehow?

(pe/coff will need to emit COMDAT symbols pointing into lexicographically sortable section names that the linker can fold away)

1446

Is there a way to ensure that objc_load happens before other static initializers across the entire set of linker inputs?

[theraven]

lib/CodeGen/CGObjCGNU.cpp

439

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?

1402

As I understand it, we can just append $m for the symbol names on PE/COFF and then create zero-size symbols with $a and $z suffixes for the end? The symbol names should be able to be the same, but if that's not possible then it would be a good idea to change them so that ELF and PE/COFF binaries look the same.

1446

This part is quite ELF-specific, and on ELF unfortunately the order of constructor initialisation is undefined (it will be in whatever order the linker decides to put them in). Running the Objective-C code first isn't necessarily correct: C++ static constructors and Objective-C +load methods could each depend on the other (and C++ on ObjC classes having been loaded).

Of course, if you have control over the linker (or possibly even a linker script) then you can adjust the ordering however you want...

[rjmccall]

Thanks, the comments help a lot.

lib/CodeGen/CGObjCGNU.cpp

439

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.)

1065

Seems much more reasonable, thanks.

1067

Micro-optimization: I'm pretty sure the type of this ternary is an r-value std::string, which means copying the string in either case; if you explicitly build a StringRef from StringName, the ternary will instead construct a StringRef and you'll avoid those unnecessary copies.

1069

The indentation here is a little misleading; I'd suggest aligning the two cases of the ternary operator.

1172

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?

[theraven]

lib/CodeGen/CGObjCGNU.cpp

439

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..

1172

This comment was inherited from the old version and is actually nonsense now. I have replaced it with one that actually makes sense.

[theraven]

Address review comments, including adding a size field for ivars.

I think I am happy with this revision and would like to commit it before working on PE/COFF support.

[rjmccall]

Thanks, my comments seem to all be addressed.

[Ka-Ka]

lib/CodeGen/CGObjCGNU.cpp

1056

The isnumber() function was added to cctype.h by Apple. I don't think it can be used in llvm.

According to
https://stackoverflow.com/questions/39204080/what-is-the-difference-between-isdigit-and-isnumber

[theraven]

lib/CodeGen/CGObjCGNU.cpp

1056

Ah, isnumber is from 4.4BSD, I assumed it worked everywhere. Changing it to isdigit is fine.

[GBuella]

lib/CodeGen/CGObjCGNU.cpp

1056

BTW isalnum is in ISO since 1989.

[GBuella]

lib/CodeGen/CGObjCGNU.cpp

1056

Also, you might want to isalnum((unsigned char)c), as passing a negative value to these functions is UB.

[xbolva00]

lib/CodeGen/CGObjCGNU.cpp

512

This causes
CGObjCGNU.cpp:589:16: warning: ‘virtual llvm::Value* {anonymous}::CGObjCGNU::GetSelector(clang::CodeGen::CodeGenFunction&, const clang::ObjCMethodDecl*)’ was hidden [-Woverloaded-virtual]

llvm::Value *GetSelector(CodeGenFunction &CGF,

[theraven]

lib/CodeGen/CGObjCGNU.cpp

512

I can't reproduce this, and I'm not sure what the issue is. The two lines have different overloads, so one shouldn't be hiding the other.

[xbolva00]

lib/CodeGen/CGObjCGNU.cpp

512

It is a warning from GCC 7.2

[theraven]

lib/CodeGen/CGObjCGNU.cpp

512

Sounds like it's a spurious one. Any idea how to silence it?

[xbolva00]

lib/CodeGen/CGObjCGNU.cpp

512

Not sure ..

virtual llvm::Value *GetSelector(CodeGenFunction &CGF, Selector Sel, const std::string &TypeEncoding); is here

but base class (CGObjCRuntime) has:

virtual llvm::Value * GetSelector (CodeGenFunction &CGF, Selector Sel)=0