Page MenuHomePhabricator

[ObjC] Fix encoding of ObjC pointer types that are pointers to typedefs
ClosedPublic

Authored by ahatanak on May 15 2019, 6:46 PM.

Details

Summary

clang currently emits different strings for s0 and s1 in the following code because pointers to typedefs are encoded as "^{PointeeClassName=#}" while ObjC pointer types that aren't pointers to typedefs are encoded as "@".

@class Class1;

typedef NSArray<Class1 *> MyArray;

void foo1(void) {
  const char *s0 = @encode(MyArray *); // "^{NSArray=#}"
  const char *s1 = @encode(NSArray<Class1 *> *); // "@" 
}

It seems that this was a deliberate choice made in r61387 to make clang compatible with gcc (see http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20081222/010465.html), but it causes apple's runtime to crash. This patch fixes the bug by checking whether the runtime is one of the GNU runtimes before using the special encoding for typedefs.

rdar://problem/50563529

Diff Detail

Repository
rC Clang

Event Timeline

ahatanak created this revision.May 15 2019, 6:46 PM
theraven requested changes to this revision.May 16 2019, 12:22 AM
theraven added inline comments.
lib/AST/ASTContext.cpp
6975–6976

Please can we at least make this check just for the GCC runtime? I'm not sure it's even needed there. I've previously had to write code that works around this and have always considered it a bug in GCC, rather than a feature that I'd like us to copy, so I'd also be happy with just deleting this code path entirely.

This revision now requires changes to proceed.May 16 2019, 12:22 AM
ahatanak marked an inline comment as done.May 16 2019, 3:57 PM
ahatanak added inline comments.
lib/AST/ASTContext.cpp
6975–6976

Are we allowed to delete the code path entirely? That would clean up the code a bit, but I assume it would also break GCC runtime users' code.

theraven added inline comments.May 17 2019, 3:35 AM
lib/AST/ASTContext.cpp
6975–6976

I'd be happy with deleting it entirely. Nothing in the gcc runtime itself depends on it and all of the code that I've dealt with that works with the GCC runtime and this functionality has explicit work arounds for this behaviour in GCC.

ahatanak updated this revision to Diff 200216.May 20 2019, 1:17 AM
ahatanak marked an inline comment as done.

Delete the entire code path that tries to work around a bug in gcc.

Delete the entire code path that tries to work around a bug in gcc.

Should something be added to the release notes for this?

Yes, I think we can add something to the release note.

rjmccall added inline comments.May 28 2019, 12:17 PM
lib/AST/ASTContext.cpp
6975

Is this option dead now?

ahatanak updated this revision to Diff 201760.May 28 2019, 2:03 PM
ahatanak marked an inline comment as done.

Remove flag EncodePointerToObjCTypedef, which is no longer needed.

LGTM. David should sign off, too, though.

theraven accepted this revision.May 29 2019, 9:05 AM

LGTM. I wonder if we have any other ugly GCC bug compatibility parts in clang's Objective-C implementation...

This revision is now accepted and ready to land.May 29 2019, 9:05 AM
This revision was automatically updated to reflect the committed changes.

LGTM. I wonder if we have any other ugly GCC bug compatibility parts in clang's Objective-C implementation...

I see a couple of FIXME comments in ASTContext.cpp that seem to suggest clang is copying gcc's behavior that is possibly incorrect.