Hi David,
Here is a test case which works now, but breaks in the Obj-C runtime
with your patch:
--
#include <objc/runtime.h>
@interface A -(void) im0; @end
SEL f0() {
SEL S[] = { @selector(im0) };
return S[0];
}
--
On Wed, Feb 3, 2010 at 3:58 PM, David Chisnall <[email protected]> wrote:
> On 3 Feb 2010, at 23:29, David Chisnall wrote:
>
>> Hi Daniel,
>>
>> How does this break the NeXT runtime? I ran the test suite and got no
>> failures, and I tested a few extra @selector() cases with -fnext-runtime and
>> got code that looked correct. If you can provide me with a test case that
>> demonstrates how this breaks the NeXT runtime then I'd be happy to fix it.
>>
>> The code generation code paths for the NeXT runtime should be exactly the
>> same before and after the patch and the only change in Sema produces the
>> warning conditionally, suppressing it when the NeXT runtime is used.
>
> Specifically, this is the only code modified in the Mac runtime:
>
>> + virtual llvm::Constant *GetConstantSelector(Selector Sel) {
>> + assert(0 && "Constant Selectors are not yet supported on the Mac
>> runtimes");
>> + return 0;
>> + }
>> + virtual llvm::Constant *GetConstantTypedSelector(
>> + const ObjCMethodDecl *Method) {
>> + return GetConstantSelector(Method->getSelector());
>> + }
>>
>
>
> This is only called in one place, in CGExprConstant:
>
>> + llvm::Constant *VisitObjCSelectorExpr(const ObjCSelectorExpr *E) {
>> + ObjCMethodDecl *OMD = E->getMethodDecl();
>> + if (OMD)
>> + return CGM.getObjCRuntime().GetConstantTypedSelector(OMD);
>> + else
>> + return CGM.getObjCRuntime().GetConstantSelector(E->getSelector());
>> + }
>> +
>
> This does change the behaviour on the Mac runtime. The change in this file
> cause it fails with an assert that CGObjCMac doesn't yet support this
> behaviour instead of 'can not codegen this constant expression'. This is now
> reached because of this change in AST/Expr.cpp:
>
>> + case ObjCSelectorExprClass:
>> return true;
>
> Which was incorrect before the change; @selector() expressions are constants
> and GCC treats them as such. The following will compile quite happily in GCC:
>
> static SEL foo = @selector(foo);
I see. This is not supported when compiling for the NeXT runtime, can
you update your patch to reflect this?
--
ddun...@giles:tmp$ cat t.m
#include <objc/runtime.h>
@interface A -(void) im0; @end
static SEL S = @selector(im0);
SEL f0() {
static SEL S = @selector(im0);
return S;
}
SEL f1() {
static SEL S[] = { @selector(im0) };
return S[0];
}
ddun...@giles:tmp$ gcc -m32 t.m
t.m:3: error: initializer element is not constant
t.m: In function 'f0':
t.m:5: error: initializer element is not constant
t.m: In function 'f1':
t.m:9: error: initializer element is not constant
t.m:9: error: (near initialization for 'S[0]')
ddun...@giles:tmp$ gcc -m64 t.m
t.m:3: error: initializer element is not constant
t.m: In function 'f0':
t.m:5: error: initializer element is not constant
t.m: In function 'f1':
t.m:9: error: initializer element is not constant
t.m:9: error: (near initialization for 'S[0]')
ddun...@giles:tmp$ clang -m32 t.m
t.m:3:16: error: initializer element is not a compile-time constant
static SEL S = @selector(im0);
^~~~~~~~~~~~~~
t.m:5:18: error: initializer element is not a compile-time constant
static SEL S = @selector(im0);
^~~~~~~~~~~~~~
t.m:9:20: error: initializer element is not a compile-time constant
static SEL S[] = { @selector(im0) };
^~~~~~~~~~~~~~~~~~
3 diagnostics generated.
ddun...@giles:tmp$ clang -m64 t.m
t.m:3:16: error: initializer element is not a compile-time constant
static SEL S = @selector(im0);
^~~~~~~~~~~~~~
t.m:5:18: error: initializer element is not a compile-time constant
static SEL S = @selector(im0);
^~~~~~~~~~~~~~
t.m:9:20: error: initializer element is not a compile-time constant
static SEL S[] = { @selector(im0) };
^~~~~~~~~~~~~~~~~~
3 diagnostics generated.
ddun...@giles:tmp$
--
> The only other change that touched Mac runtime code paths was this:
>
>> + ObjCMethodDecl *OMD = E->getMethodDecl();
>> + if (OMD)
>> + return CGM.getObjCRuntime().GetSelector(Builder, OMD);
>> + else
>> + return CGM.getObjCRuntime().GetSelector(Builder, E->getSelector());
>
> This calls a method that was already present in the GCObjCMac (calling
> EmitSelector() with OMD->getSelector() as the argument, so this will generate
> exactly the same code for both code paths on the Mac runtime).
>
> The only other change was to add a ObjCMethodDecl to the ObjCSelectorExpr.
> This change has no semantic effect on the Mac runtime - indeed, the test for
> the Mac runtime here was added specifically because an earlier version of the
> patch did fail a test because a warning that the Mac runtime didn't expect
> was generated.
>
> So, I am at a loss to see where this broke the Mac runtime aside from causing
> it to fail in a different place when you try to compile (valid) code which
> uses an @selector() expression as a static initialiser. I'm sorry if it
> broke the Mac runtime, but I did test it with all of the tests in the current
> test suite (and got no unexpected failures) and with a few extra programs
> using -S -fnext-runtime -emit-llvm to see if it still generated the same code
> as before the patch.
Fair enough. Our test suite is very limited, this is an unfortunate reality.
> 'Substantially breaks working code' is possibly the least useful bug report
> that I have ever read. I did everything in my power to ensure that this did
> not affect the Mac runtime, but if it did then I would appreciate it if you
> would provide me with some hints as to how it broke the Mac runtime, rather
> than just asserting that it did.
Sorry, your commit message made it sound like you were already aware
of the issue but had committed it anyway (hence my response). I
understand now you thought it was just enabling code that already
didn't work. I still think that:
(a) it definitely should come with a test case,
(b) it would have made sense to send as a PATCH, even just as a
heads up since it *could* impact the NeXT runtime and you don't have
the ability to test it.
- Daniel
>
> David
>
> -- Sent from my Apple II
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
