Page MenuHomePhabricator

[AST] Revert mangling changes from r339428
ClosedPublic

Authored by smeenai on Sep 26 2018, 5:29 PM.

Details

Summary

As discussed in https://reviews.llvm.org/D50144, we want Obj-C classes
to have the same mangling as C++ structs, to support headers like the
following:

#ifdef __OBJC__
@class I;
#else
struct I;
#endif

void f(I *);

since the header can be used from both C++ and Obj-C++ TUs, and we want
a consistent mangling across the two to prevent link errors. Itanium
mangles both the same way, and so should the MS ABI.

The main concern with having the same mangling for C++ structs and Obj-C
classes was that we want to treat them differently for the purposes of
exception handling, e.g. we don't want a C++ catch statement for a
struct to be able to catch an Obj-C class with the same name as the
struct. We can accomplish this by mangling Obj-C class names differently
in their RTTI, which I'll do in https://reviews.llvm.org/D47233. I would
have done the same for the GNUstep RTTI here, except I don't actually
see the code for that anywhere, and no tests seem to break either, so I
believe it's not upstreamed yet.

Diff Detail

Repository
rC Clang

Event Timeline

smeenai created this revision.Sep 26 2018, 5:29 PM

I would have done the same for the GNUstep RTTI here, except I don't actually
see the code for that anywhere, and no tests seem to break either, so I
believe it's not upstreamed yet.

I'm not sure I understand this comment. The compiler code is in LLVM and the tests that you've modified are the ones that would fail. The corresponding code is upstreamed in the runtime, to generate EH metadata for the throw with the same mangling. If you are going to change the mangling, please make sure that the existing mangling is preserved for EH when compiling for the GNUstep / Microsoft ABI on Windows.

I'm also deeply unconvinced by the idea that it's okay to have a struct I in one compilation unit and a @class I in another. The struct version will not do any of the correct things with respect to memory management. Code that has this idiom is very likely to be broken.

I would have done the same for the GNUstep RTTI here, except I don't actually
see the code for that anywhere, and no tests seem to break either, so I
believe it's not upstreamed yet.

I'm not sure I understand this comment. The compiler code is in LLVM and the tests that you've modified are the ones that would fail. The corresponding code is upstreamed in the runtime, to generate EH metadata for the throw with the same mangling. If you are going to change the mangling, please make sure that the existing mangling is preserved for EH when compiling for the GNUstep / Microsoft ABI on Windows.

I'm also deeply unconvinced by the idea that it's okay to have a struct I in one compilation unit and a @class I in another. The struct version will not do any of the correct things with respect to memory management. Code that has this idiom is very likely to be broken.

Ah, it looks like it's sharing the C++ RTTI codepath. What I meant by tests specifically was ones like in https://reviews.llvm.org/D47233, where you're explicitly checking the RTTI generated for catch handlers. None of the tests that I'm modifying in this diff provide that sort of coverage, and no other tests break, so we don't have that coverage at all.

The header idiom is used by WebKit among others (https://github.com/WebKit/webkit/blob/d68641002aa054fd1f5fdf06d957be3912185e14/Source/WTF/wtf/Compiler.h#L291), so I think it's worth giving them the benefit of the doubt.

I'm assuming https://github.com/gnustep/libobjc2/blob/master/eh_win32_msvc.cc is the corresponding runtime code. I'll adjust the mangling of RTTI emission accordingly, but I also really think you should explicitly add regression tests in clang itself for the RTTI emission, otherwise it's liable to break in the future as well. (I'll try to add a few basic regression tests if I have the time.)

smeenai added a subscriber: rnk.Sep 27 2018, 11:56 AM

Adding @rnk, since this'll touch MS ABI mangling. For context, we want struct X to have the same mangling as @interface X normally, but we want to be able to distinguish them for the purpose of exception handling. See this diff's summary for more details.

The simplest option is something like P8109, where we add a .objc discriminator when mangling the RTTI itself. It would require the GNUStep runtime for Windows to be altered accordingly (e.g. https://github.com/gnustep/libobjc2/blob/master/eh_win32_msvc.cc#L85 would have to use ".objc.PAU" instead of ".PAU.objc_cls_"); would that be acceptable, @theraven and @DHowett-MSFT?

If we want to keep the Obj-C discriminator as an infix instead of a prefix, we'll probably have to keep some state around in the mangler indicating if we're mangling for RTTI or not. That seems ugly, but I don't know if there's any other way to do it.

The simplest option is something like P8109, where we add a .objc discriminator when mangling the RTTI itself. It would require the GNUStep runtime for Windows to be altered accordingly (e.g. https://github.com/gnustep/libobjc2/blob/master/eh_win32_msvc.cc#L85 would have to use ".objc.PAU" instead of ".PAU.objc_cls_"); would that be acceptable, @theraven and @DHowett-MSFT?

I think that's fine. We have shipped the new ABI for ELF platforms, but not yet for Windows (and we have a few other patches still needed in Clang), and haven't finished the WinObjC integration, so we don't yet have a stable ABI that anyone cares about preserving for this - we explicitly disabled the Windows code paths in clang 7.0, so no one can use them with an unstable ABI without patching clang. As long as we come up with some consistent mangling and it's sensibly documented, I'm happy to make the runtime changes for it.

D52674 adds the RTTI Obj-C discriminator. I put it up as a separate change so it could be reviewed independently and more thoroughly.

rnk accepted this revision.Oct 1 2018, 2:23 PM

I think going back to the old mangling makes sense.

Regarding memory management, I assume that the C++ code is just forward declaring objective C class and interface types so it can pass them around by pointer. It's not going to do ARC, obviously, but it should be safe enough.

This revision is now accepted and ready to land.Oct 1 2018, 2:23 PM
This revision was automatically updated to reflect the committed changes.