[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

[David Chisnall]

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

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.  

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

David

-- Sent from my Apple II