[cfe-commits] r95189 - in /cfe/trunk: include/clang/AST/ExprObjC.h lib/AST/Expr.cpp lib/CodeGen/CGExprConstant.cpp lib/CodeGen/CGObjC.cpp lib/CodeGen/CGObjCGNU.cpp lib/CodeGen/CGObjCMac.cpp lib/CodeGen/CGObjCRuntime.h lib/Sema/SemaExprObjC.cpp

[Daniel Dunbar]

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 <csdavec at swan.ac.uk> 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?
--
ddunbar at 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];
}
ddunbar at 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]')
ddunbar at 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]')
ddunbar at 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.
ddunbar at 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.
ddunbar at 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