Fixing super lookup in class methods (GNU ObjC runtime)

Hi,

This patch fixes message sends to super in class methods for the GNU runtime (currently an instance method lookup is being performed).

David

clang.diff (682 Bytes)

Applied, thanks!
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090413/015828.html

-Chris

And reverted here. This patch broke test/Coverage/codegen-gnu.m, one of the few tests we have for the gnu runtime codegen. Please make sure that your patches pass basic sanity tests before submitting them, thanks.

-Chris

This test failure is due to a bug in Sema. Under the comment:

// Synthesize "typedef struct objc_object { Class isa; } *id;"

It is synthesising "typedef struct objc_object *id", i.e. synthesising id as a pointer to an opaque type. This is incorrect (and I'm fairly sure a recent regression). The original patch works correctly when id has a valid type (i.e. one which contains a pointer to the class), which is the case when any of the standard Objective-C headers is included and will be the case when this bug in sema is fixed.

I can work around this in CGObjCGNU by adding an explicit bitcast to a { i8* }*, but that seems like an ugly hack.

David

David,

I don't believe this is a recent regression.

The struct body was removed ~10 months ago (6/21/08), when the code moved from Sema::Sema to Sema::ActOnTranslationUnitScope.

I don't see why you think this is incorrect. It doesn't seem to be biting anyone else...

snaroff

[steve-naroffs-imac-2:~/llvm/tools/clang] snaroff% svn info -r52593
Path: trunk
URL: https://llvm.org/svn/llvm-project/cfe/trunk
Repository Root: https://llvm.org/svn/llvm-project
Repository UUID: 91177308-0d34-0410-b5e6-96231b3b80d8
Revision: 52593
Node Kind: directory
Last Changed Author: lattner
Last Changed Rev: 52593
Last Changed Date: 2008-06-21 16:20:39 -0400 (Sat, 21 Jun 2008)

Index: test/CodeGen/empty-union-init.c

Hi Steve,

I don't believe this is a recent regression.

The struct body was removed ~10 months ago (6/21/08), when the code
moved from Sema::Sema to Sema::ActOnTranslationUnitScope.

No, it's not recent - I didn't notice it when it was committed.

I don't see why you think this is incorrect. It doesn't seem to be
biting anyone else...

It is incorrect for two reasons:

1) The code does not match the comment (not a huge problem, but at the very least the comment should be changed to reflect what the code is actually doing).

2) The code does not define id as a valid type for an object pointer.

I suspect it is not biting anyone else because this implicit typedef is only ever used in Objective-C programs that do not import the standard header files, which is a set which does not include any Objective-C programs in the real world. When I tested the diff that was committed and since reverted, I only tested it on real code that was experiencing the problem of messages to super not working correctly. By reverting the diff, we have gone from having real code working but the test failing to having real code and the test both generating incorrect code which will fail at run time, but the test working passing. To me, this does not seem like an improvement.

As I said, I can work around this change in CodeGen by explicitly casting the id to a type which is guaranteed to be valid, but it seems like an ugly hack and a better fix would be to ensure that an id is always a pointer to a struct containing a pointer to a class (which is opaque unless declared otherwise in a header).

David