[PATCH] D52581: [AST] Revert mangling changes from r339428

Thu Sep 27 09:51:27 PDT 2018

smeenai added a comment.

In https://reviews.llvm.org/D52581#1247409, @theraven wrote:

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

Repository:
  rC Clang

https://reviews.llvm.org/D52581